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

refactor: handle "non"-validating output for async actions #1279

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

danielpeintner
Copy link
Member

fixes #1278

@danielpeintner danielpeintner marked this pull request as draft April 29, 2024 13:37
@danielpeintner
Copy link
Member Author

I looked at how we can re-work InteractionOutput so that we do not throw Error: Invalid value according to DataSchema for async action output.
The problem is that InteractionOutput does not know whether we deal with property/action/event nor whether the action is synchronous.

I see some possible solutions:

  1. Extend InteractionOutput with information whether we deal with action and information about synchronous flag
  2. Extend InteractionOutput constructor with the full interaction object (maybe useful in future?)
  3. Extend InteractionOutput constructor with the hint whether validation should take place (most generic)

What do people think? Any other ways to solve the problem?

@danielpeintner danielpeintner marked this pull request as ready for review April 29, 2024 14:19
@danielpeintner
Copy link
Member Author

I extended InteractionOutput so that one can pass ignoreValidation flag. By default validation is enforced (as it was before).
For actions ignoreValidation is set to TRUE unless synchronous flag is true.

@egekorkan
Copy link
Member

I think changing the option to ignore validation is good but I am not sure about the async/sync action difference. Where do we say that async action's response is not the output? From my pov, it is the responsibility of the developer to make the difference and the output should contain the response to the invocation.

@danielpeintner
Copy link
Member Author

I think changing the option to ignore validation is good but I am not sure about the async/sync action difference. Where do we say that async action's response is not the output?

The TD spec states the following

Lack of this keyword (synchronous) means that no claim on the synchronicity of the action can be made

see https://w3c.github.io/wot-thing-description/#table-vocabulary-terms-in-actionaffordance-level

This tells me that if the keyword synchronous is missing it can be either. Hence enforcing the validation might cause issues since the value reported might be the ActionStatus object rather than the output.

@egekorkan
Copy link
Member

There is no such thing as an "ActionStatus" object. It only exists in one profile. If that is a generic mechanism, it should be in the TD spec first. The main unclear part in the TD spec is the meaning of output "Used to define the output data schema of the Action.". Is output the result of the physical process? Is it protocol level response?

@danielpeintner
Copy link
Member Author

There is no such thing as an "ActionStatus" object

I know. I used "ActionStatus" object as the term since it used in the profile and known by everyone. What I mean in general is that async operations do not send the value directly after calling invokeAction() and may report something (or nothing) after the invocation and the actual value will be reported in some other fashion later.

Hence, the output (for async cases) is not what we can expect and therefore no validation should take place. That's all what the current PR is about.

@egekorkan
Copy link
Member

I understand but what if the intention is that output field matches the actionstatus object? For now, we can go with this direction but I think there is no consensus on this from the standardization perspective.

@relu91 relu91 added the Discuss in Committer Meeting Labels issues and PRs that should be discussed in our regular Thingweb Committer Meeting. label May 3, 2024
@danielpeintner
Copy link
Member Author

I understand but what if the intention is that output field matches the actionstatus object?

Mhh, that sounds weird to me but I see now where you are coming from.

Having said that, TD2.0 should have a way to describe both, the final output of an action and the "output" provided after invoking the action.

@danielpeintner
Copy link
Member Author

Any more opinions on the PR? Is it the right way forward for now?

@relu91
Copy link
Member

relu91 commented May 6, 2024

We had a casual chat in the last committer meeting about this PR. One thing that we can improve is to introduce an options object as the last argument of InteractionOutput constructor. It should be a little more flexible for future improvements. Then we can expose that option in the InteractionOption object of each affordance once we solve #1282. Therefore, it might have the potential to solve (as a workaround) a bunch of different use cases.

@danielpeintner
Copy link
Member Author

danielpeintner commented May 7, 2024

One thing that we can improve is to introduce an options object as the last argument of InteractionOutput constructor.

An "option object" makes sense 👍

What I am not very sure about when reading your comments is

  • whether the "option object" should be of type InteractionOptions? I don't think so since it defines formIndex, uriVariables and data only? (we could extend it like this)
  • If we use another "option object": should we properly define flags like ignoreValidation or do you want to keep the object open as any. I wouldn't like to do that. It makes it difficult to do proper (type)checking

Once we can agree on the possible options I can update the PR accordingly.

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.

Another minor point. I am not sure to leave the logic to automatically ignore validation if the action is asynchronous give @egekorkan's points. But we can remove it once we proper expose this ignoring feature to the end user.

@@ -122,14 +124,14 @@ export class InteractionOutput implements WoT.InteractionOutput {
// validate the schema
const validate = ajv.compile<T>(this.schema);

if (!validate(json)) {
if (!this.ignoreValidation && !validate(json)) {
Copy link
Member

Choose a reason for hiding this comment

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

I would skip schema compilation since in the case of ingnoreValidation === true is completely useless.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mhh, in the case of ignoreValidation === true the part after && is no longer taken into account since the if cannot turn into true anymore?

Or did I misunderstand the comment?

@danielpeintner
Copy link
Member Author

Another minor point. I am not sure to leave the logic to automatically ignore validation if the action is asynchronous give @egekorkan's points. But we can remove it once we proper expose this ignoring feature to the end user.

@relu91 @egekorkan
I felt it is safer to not validate action output if we don't know exactly whether the output can/should be validate.
Please let me know what we should do for now since users cannot turn off the validation for now. IF it were a warning only that would be fine by me .. but refusing to work just because "we" think we should do validation doesn't sound correct to me 🙈

@egekorkan
Copy link
Member

egekorkan commented May 13, 2024

I think it is even better to turn of validation by default. Regarding the compilation of the schema or not, it is better not compile it but I am not sure if the JS engine respects the order and does not compile the second. It would be cleaner to check ignoreValidation first and then go the validation in another if branch?

@danielpeintner
Copy link
Member Author

danielpeintner commented May 13, 2024

I think it is even better to turn of validation by default.

Turning it off for actions only all for all interactions like properties also?

Mhh. We do have this for a very long time now and I do not fully understand why we disable it now (since a user is not possible to configure it).

My take is that for property and event I would keep it. For action I would keep it also, if we know the action is synchronous.

Regarding the compilation of the schema or not, it is better not compile it but I am not sure if the JS engine respects the order and does not compile the second. It would be cleaner to check ignoreValidation first and then go the validation in another if branch?

I feel the opposite. It is surely the case that all following statements in a && sequence are not executed if a previous one is false (JS uses uses short circuit evaluation).

@relu91
Copy link
Member

relu91 commented May 15, 2024

I would not default to no validation either. I think we should discuss these changes in the next commiter meeting to better understand what to do next. There is also the fact that actually, users have already some options to avoid validation. I list some here that we can review.

Defining a utility function getValueNoMatterWhat

async function getValueNoMatterWhat(output: InteractionOutput): Promise<unknown> {
   try{
     return await output.value()
   } catch(error){
      if(error.value != null){
         return error.value
      }
      throw error
   }
}

Use arrayBuffer function

In the code instead of calling value they can use the arrayBuffer function as follows:

const output = await thing.invokeAction("test")
const value = JSON.parse(Buffer.from(await output.arrayBuffer()).toString())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discuss in Committer Meeting Labels issues and PRs that should be discussed in our regular Thingweb Committer Meeting.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Async actions should not try to validate the value according DataSchema
3 participants