Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(InteractionOutput): don't require schema.type in value function #534

Merged
merged 7 commits into from
Mar 4, 2024

Conversation

JKRhb
Copy link
Member

@JKRhb JKRhb commented Jan 22, 2024

At the moment, the value function of the InteractionOutput interface requires the type field of its schema to be set. In a discussion over at eclipse-thingweb/node-wot#1221, we identified this as a problem for several different scenarios, including the exloreDirectory method, as the things property of the Thing Description Directory API specification does not contain a type field itself, as there are two different types that can be obtained from the property (namely, an array or an object of TDs).

Therefore, this PR proposes changing the algorithm to not throw anymore when the field should be null or undefined, making it usable in more scenarios.


Preview | Diff

@JKRhb JKRhb requested review from danielpeintner, relu91 and zolkis and removed request for danielpeintner January 22, 2024 21:58
Copy link
Contributor

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the discussion we had in eclipse-thingweb/node-wot#1221, and the arguments provided it makes sense to soften the restriction a bit

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I probably failed to express this correctly, but we need the data schema in the value function, and without it the value function does not make any sense... it is like mapping from A to B without B. Hence, as I said in this comment, the algorithm should say:

... or if |schema| is null or is not a valid schema then reject...

Problem is that we need to define what is a valid schema for us. We got around this because so far for scripting API the schema did not have complex root types (like oneOf, allOf etc.). See also this section of how to use the schema, it is very simplified but in reality (also in node-wot) schemas are much more complex than that.

@JKRhb
Copy link
Member Author

JKRhb commented Jan 23, 2024

Very good points, @relu91, thank you. You are right, this definitely requires more work, especially when it comes to the "check data schema" algorithm. Could we maybe refer to something already defined by JSON Schema here?

Comment on lines +979 to +980
|dataUsed| is `true`, or if |form| is not an {{object}} or if |schema|
is `null` or `undefined`, then
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, this is (more or less) the change proposed by @relu91 in #534 (review). The question here, also raised by him, is how we should ensure that the schema is valid – do we need to define a separate algorithm for that?

Suggested change
|dataUsed| is `true`, or if |form| is not an {{object}} or if |schema|
is `null` or `undefined`, then
|dataUsed| is `true`, or if |form| is not an {{object}} or if |schema|
is `null` or `undefined` or not a valid schema, then

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: JSON schema is VERY loose when it comes to what is a valid schema.. almost anything is a valid JSON schema. Hence I am not sure if it makes sense to do anything on top...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah, I now remember that we also discussed this briefly on Friday during the Thingweb meeting... Then I suppose we should keep this step as is, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"not a valid schema" needs an algorithm

@relu91
Copy link
Member

relu91 commented Feb 5, 2024

Call 05/02:

  • @JKRhb relates to the todo item of DataMapping, so that's why it is hard to fix without knowing the result of the discussion
  • @danielpeintner we can try a best-effort approach and suggest proper schema validation in an Editor's note
  • @relu91 agrees, we can modify the algorithm to support the following keywords that are defined in the TD:
    1. oneOf
    2. const
    3. enum
    4. default ? (related to Proposal: use TD default values #537)
  • @ashimura It would be important to describe the use cases of each one of these keywords. @relu91 I agree let's open an issue before merging this PR.

@JKRhb
Copy link
Member Author

JKRhb commented Feb 12, 2024

I've now tried to incorporate oneOf, enum, default, and const into the check data schema algorithm in 0f00764. There are at least two aspects of the algorithm that we could discuss in the call later today – I marked these with TODO comments in the HTML document.

@JKRhb

This comment was marked as resolved.

@relu91
Copy link
Member

relu91 commented Feb 19, 2024

Call 19/02:

  • @ashimura we need to gather more implementation feedback because the issue might be related to other problems in different specifications.
  • @relu91 I agree but for now I would prefer an iterative approach and fix the issue with what we know ( we can keep Should schema.type be always defined?  #546 to continue the discussion).

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@relu91
Copy link
Member

relu91 commented Mar 4, 2024

From 04/03:

  • @relu91 changes look good (+1 for removing default), they might not be perfect but they can refined later
  • @danielpeintner approves the changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants