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

[danielpeintner]

fixes #1278

[danielpeintner]

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]

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]

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]

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]

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]

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]

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.

[danielpeintner]

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]

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

[relu91]

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]

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.

[relu91]

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]

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

[danielpeintner]

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]

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]

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]

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]

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())

[egekorkan]

I think that's also fine. We should just document this approach. If we can log and point to documentation in case of validation failure, even better but it might be too verbose in logs

[relu91]

After a loooong discussion and earing all the different points of view. We have chosen to go with adding a parameter to the value function to turn off validation. This will be useful for other use cases as well and has the advantage not to be verbose as const value = JSON.parse(Buffer.from(await output.arrayBuffer()).toString()).

For the future, invoking an async action might return a different object (not an InteractionOutput) where the application can call methods like queryaction and cancelaction operations.

[danielpeintner]

FYI: I created 2 Scripting API issues: w3c/wot-scripting-api#554 and w3c/wot-scripting-api#555

[danielpeintner]

Since I expect some time to pass before the proposed changes may land in the Scripting API. Do we still want to "fix" #1278 temporarily in one way or the other?

[relu91]

Given the discussion of today's committer meeting:

• TD spec is not clear about what to expect when the sync property is missing
• Eargerly validate the output in this cases means to try out smart the developer which is quite a bad practice
• Therefore we decided to merge this PR as a workaround for the whole situation. We acknowledge that there needs to be follow up fixes both in Scripting API and Thing Description spec to properly solve the issue.