Improve TD validation error in requestThingDescription

[danielpeintner]

fixes #1403

[danielpeintner]

Unfortunately, it is not possible to wrap an error in TS/JS.

[relu91]

Unfortunately, it is not possible to wrap an error in TS/JS.

You should be able to define custom errors.

export class ValidationError extends Error {

  public constructor(input: ??) {
    //... 
  }
}

or did I miss your point?

[danielpeintner]

What I meant was wrapping this error in another error to get a stack trace like in Java

[egekorkan]

But this still makes me think that there is a TD validation error. The request method should be catched?

[relu91]

Yeah, in the case you described in the issue, the client.requestThingDescription should fail... the problem is here:
https://github.com/eclipse-thingweb/node-wot/blob/master/packages/binding-http/src/http-client-impl.ts#L249

We are not checking the result (e.g., 404 or any other error condition).

Unless the URL you provided actually answers with a JSON payload... in that case the error is correct IMHO.

[danielpeintner]

The issue happen in the following code

node-wot/packages/core/src/wot-impl.ts

Lines 99 to 103 in 3325e12

	if (isThingDescription(value)) {
	return value;
	}
	throw getLastValidationErrors();

Line99 returns false and hence Line103 throws the TD validation issue.
In @egekorkan case it just says "Error: must be object" which is not clear "what" should be an object.

What I do in my PR is essentially just extending the error (with "TD validation error: ") so that it is clear what the error is about. In this case, the TD is not valid.
Not sure what you expect me to do...

[egekorkan]

In this case, the TD is not valid.

No in this case there is no TD to begin with. The link @relu91 sent makes sense as there is no handling of request not resolving

[danielpeintner]

How do I know whether the result is a TD? We have the isThingDescription(...) call for that, right.

In your case, the issue is not that we get a 404 (in such a case you get a 404 message). The issue is that you get a result .. in this case, a JSON array as response...

[relu91]

The issue is that you get a result .. in this case, a JSON array as response...

As I said above, if this is the case, the answer @egekorkan described in the issue is indeed correct.

[danielpeintner]

I seem to be on the wrong track somehow!? 🤷‍♂️

Can you explain to me what you ask me to do? Or do you want to propose a fix?

Yes, we get an array as response, and I do know that this can never be a TD.
Sure, I can add a type check .. but why should I do that in the node-wot code if the code we are calling ( i.e., isThingDescription(...)) is doing that already.

Hence, I only add to the error message we are getting the information that the error is caused be the TD validation... so that developers better understand what the error is about.

[relu91]

I seem to be on the wrong track somehow!?

No, you are correct! I was saying that since we are receiving a valid json (and not a 404 or other error codes like a timeout) the error that we are throwing is correct, we can improve as you are doing here, but I would not even bother (but we can move on if you like).

[danielpeintner]

I will take a closer look at the possible error cases and look also into client.requestThingDescription(url) and check whether a HTTP status >= 400 throws an error also

[danielpeintner]

Commit 9afaaf2 should add the information needed if an HTTP error happens...

[danielpeintner]

I think this brings up a new issue

https://github.com/eclipse-thingweb/node-wot/actions/runs/15978405985/job/45066738871?pr=1404#step:9:310

I think it should be handled by the following part already but it doesn't seem to be the case

node-wot/packages/binding-http/src/http-client-impl.ts

Line 433 in 699e6b6

if (HttpClient.isOAuthTokenExpired(result, this.credential)) {

[danielpeintner]

I think it should be handled by the following part already but it doesn't seem to be the case

node-wot/packages/binding-http/src/http-client-impl.ts

Line 433 in 699e6b6

if (HttpClient.isOAuthTokenExpired(result, this.credential)) {

It is not a OAuth error but a generic unauthorized error... I added a special case for it to make the following test happy.

node-wot/packages/binding-http/test/http-client-basic-test.ts

Line 115 in 699e6b6

error.message.should.eql("Client error: Unauthorized");

[danielpeintner]

Anything missing?
@relu91 @egekorkan

[relu91]

LGTM, as discussed in call. Let's merge it.