-
Notifications
You must be signed in to change notification settings - Fork 78
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
readOnly properties cannot be internally updated? #333
Comments
I believe this is a bug in the script and not in the node-wot implementation. We also had discussions in the past (see w3c/wot-scripting-api#106) and node-wot now enforces a check (and refuses) before writing a value that is Let's assume we have the following TD for a property
The node wot runtime can not detect whether a @egekorkan @relu91 any opinion? I believe there are other scripts out there that used it before and will fail now. |
I think that this makes more sense however this is a breaking change, i.e. now I have to rewrite all my Thing implementations :( Such changes require a more significant version update than incrementing the patch number |
We can also "hold back/revert" this change till v0.8.x is ready (which will be soon) ... The overall question remains whether this is how it is meant to work. |
I think if the designer decides that it is a |
I also think that they should not be writable by outsider Consumers but it is really convenient to be able to write to properties internally instead of needing to create variables and manage them. This breaking change even requires me to write extra read handlers if I am reading values from a sensor every second and storing them in a property. |
However, there is no need to fix it if there will be no npm publishes until the version 0.8.x |
Writing a property is implemented internally in the implementation script. |
@egekorkan this is already part of published node-wot v0.7.2 (see PR #328) |
If we do this, how can we emit observable property "events"? Since the new property value is emitted when it is written to, should an implementation come up with its own way of emitting a new property value? |
Not sure if I get the use case. I see 2 use cases.
In the ExposedThing script, the
If there is a Thing that exposes a read-only Property, the API contract is that writeProperty() must fail on it, full stop. But that does not necessarily mean a client cannot subscribe to value changes on that Property. |
So I am more on the first use case you describe, I think. Here is an example Thing implementation that has an observable property: On line 85, once the infrared sensor detects an object, the value is written to the property and this line alone results in the emission of an observed property event. How can this be achieved without leaving the Scripting API context if the property cannot be written to in the same script that exposes it? |
I think it still fires notification of value changes. See https://github.com/eclipse/thingweb.node-wot/blob/8ee006a971e45dded6cf2d7df0256dec512b5eb9/packages/core/src/exposed-thing.ts#L244 where it fires this event even if a handler is used or do I miss anything here? |
It can be written by the local methods of the script, if the matching entity is locally writeable. Where the writing SHOULD fail is all the incoming write requests to that Property, i.e. all the remote client Currently the API spec doesn't have steps for handling readonly enforcement, that is currently an implementation detail, since the script doesn't know whether a Property has a local binding (via a library) or is accessed via a network interface. |
keep issue open for discussions to settle |
I agree with this and it still would fire if it could be written to locally. I totally agree that anything coming over the network should not be allowed to write to a readOnly property |
Some further comments:
This part is never triggered since the following check is valid for a readOnly property, thus the reject happens on the following line:
|
I agree that we need to solve this. I tempted to go with the first solution: Having said that I think we should go with the second option: add a |
Yes, I agree that it is confusing that such a way exists. One may never notice that this is possible since you would be used to not use writeProperty for readOnly properties. Regarding In any case, a solution is needed :) |
ExposedThing implements all its methods, including writeProperty, and as I said above, they are basically local private methods. I am even thinking to remove the inheritance and its spec, as it seems confusing. They are all local convenience methods, not really needed. |
I guess "what" we try to achieve is to differentiate between the I wonder how we can achieve that. What could be the best means to differentiate the two cases?
So far I can't really come up with a good proposal either :-( |
The idea with ExposedThing is that a script implements it. This will not break any existing code, since local/private ConsumedThing functions could be still implemented on ExposedThing. Only that we'd not specify it that way. It's a relatively minor change in the spec. |
Totally agree here. However, I think the current API lacks the means to convey that a property value has changed. Therefore, we can't really implement an ExposedThing. Currently, it was done implicitly by the writeProperty method, but I do not think this is the right approach. I propose to add this new method: interface ExposedThing {
async emitPropertyChange(property:string):void;
} Basically, we don't duplicate the Once we have this new method, we could enforce the fact that if a property is a What do you think? p.s. to me this is pretty similar to what is done in other APIs like Observers in Java and .Net propertyChanged events. |
I am almost totally fine with the above mentioned solution. One thing that bugs me is that before it was inherently assured that the property changes were emitted. Now, it would be up to the developer to do that. It might be better since maybe you don't want to emit every change but want to change the internal value very often. In any case, the way it was introduced in a minor version is breaking a lot of my stuff that used the ^ notation for versioning. |
I assumed that part could be encapsulated by the implementation, once it has the list of Property observers.
Versioning of the Scripting API (from the spec, propagated to implementations) is under discussion, your suggestions would be welcome on that. |
That's precisely why I made my proposal.
I guess we could still live with ConsumeThing -> ExposedThing inheritance. I still find it convenient to pass around an ExposedThing instance to methods/functions that expect a ConsumeThing. But if others can't stand it we could drop the relation.
I think @egekorkan was talking about node-wot versioning. I think the problem was that after the #328 we increased the patch number of node-wot version. This broke every Ege's script since they were dependent to ^0.7.0 (which basically means every version with 7 as minor). @egekorkan correct? I guess the problem right now is that we can't increase the maior because we are working on the 0.8.0 as the version with the new API. When we finally have the new apis we have more room to change the version number. @danielpeintner correct?
Anyhow, this issue is still standing. |
Yes :) By the way, for the |
We should think about it, but I feel like it does not need a value. The platform should use the read handler defined for that property and get the value back from there. No change to the value should happen here. Then the runtimes MAY (?) cache previous values and send the event only if a real change happened. |
We could use the TD instance of an ExposedThing and create a ConsumedThing from that. Basically we end up with the same thing, but a bit more separated and better defined. |
That might depend on the binding, too. In OCF/CoAP binding, on the client side, the property change notification must indeed provide the value as well. Next reads would return the same value, until the next change. We can handle this the same way we handle a read, i.e. using the |
Ah ok, so if I get it correctly, I would need to do something like the following: var myTemp = 25;
thing1.setPropertyReadHandler("temperature", () => {
resolve(myTemp);
});
thing1.setActionHandler("increaseTemperature", () => {
myTemp = myTemp +2;
emitPropertyChange("temperature"); //actually emits since the value returned by myTemp has changed
resolve();
}); It is not bad but the developer needs to remember the relation between |
You could add a convenience function, thing1.updateTemperature = (value) => {
await updateLocalTemperature(value);
await emitPropertyChange("temperature", value);
}; and use |
@egekorkan the example looks good to me and I think the shortcomings that you mentioned are not that bad. |
Meanwhile we are discussing to remove |
Comments w.r.t. to the example given by Ege here I think in the case of using built-in value containers it should be much simpler. Hence, if no user-defined write handler is set we should be able to just say say Once a dedicated write handler is set the task of doing the right calls is shifted to the developer. Having said that, I think we might want to differentiate the 2 cases... |
I think Ege was thinking about the |
Summarizing from the Scripting call today (Nov. 9. 2020),
|
These are good improvements! :) |
About point 3, I do have some reservations. The request handlers are already meant to provide the values from sensors and are more generic (don't only provide the value, but also define how to handle the requests). For an ExposedThing to work, the essential minimum is to provide the request handlers. Managing internal properties can be done locally by the script. One argument for the internal accessor interface was to make scripts more portable. However, I would argue the sensor specific parts of the script need to be ported anyway, so standardizing the trivially small part that writes values to internal slots/containers (also assuming there are internal containers, which is a local design decision) would not help much IMHO, it just introduces more complexity in the spec. Right now, if a request handler is not defined, the implementation will signal error (OK, it could also implement a default handler, but currently we force it to return an error since the lack of understanding suitable default values, which depend on the semantics of the properties). Internal slots for properties may be optional (node-wot does implement them, others MAY skip that and provide values "on the fly"). But if we wanted that implementations could provide default handlers, we could do that with 1+2 as well. If we introduce 3, the local accessor API + their associated mechanisms in ExposedThing, implementations SHOULD have internal slots for properties at least, and then scripts will have to follow a certain pattern. We could go as far as saying that if the script implements the internal updating of a property, and it doesn't want to define a special request handling, then it could skip defining the read request handler for that property, since all the implementation needs to do when a read request comes is to read the internal value (that was previously updated by the script) and return it in the reply. That is, implementations will have a default handler for Properties read requests, which invokes the script-defined request handlers (if defined) in order to provide the values, then in success case, update the internal slot with that value and reply to the request. In the case an update is made to an internal slot, it could automatically notify the listeners (making the need for So the programming style changes from an all-explicit scripting towards a "script only the special cases for properties" approach. That is quite a big change. Also note that all this is only valid for Properties. Events may also carry data, but that is governed by the EventListenerHandler, similarly as for properties is now governed by PropertyReadHandler. Which makes the current API (1+2, without 3) simple (no default background mechanisms to understand and keep in mind) and coherent across handling Property, Event and Action interactions. But 1+2+3 (or 2+3) would make it far easier to create SW defined ExposedThings. Basically we decided in the call to do only 1 and 2 for the moment. Please join the next Scripting call if you want to discuss 3 further. These are 2 different programming styles and developer feedback would be needed in order to move forward. |
Let's go with it! 👍 . About point 3 (and actually the current implementation) I also have my concerns. Having the ability to store property values inside an ExposedThing instance might be convenient but it adds quite a lot of complexity to the runtime level. It is as if the runtime wants to predict how the properties should be served and also stored. In the end, this might lead to inefficiencies, like in one of our applications. We end up having for a property to have our own variable that keeps tracks of property values, plus the one inside the exposed thing. To have a better separation of roles I'll implement this caching/stateful(?) feature on a different framework/library. I think our goal is to make the API as flexible/simple as possible so that developers could extend it to meet their own requirements. Therefore, an ExposeThing implementation should look like what is shown in @egekorkan 's comment. |
That would make sense. It could be imported in a script and enjoy the convenience without cluttering the minimal Scripting spec. |
TLDRSorry, the post is pretty long. Summarizing, I am describing another edge use-case to have a ConsumeThing from an ExposedThing. This one is more about using an ExposedThing rather than defining it. I think could give it a try after we close point 1 and 2. Another use-case for ConsumedThing in ExposedThingNow about the whole Local interaction with an Exposed Thing subject, let me clarify the use case I had in mind in today's call. It is not related on how we define an ExposedThing but rather on how we can interact with it from the same script. Let's assume that we have implement point 1,2 above. So we can't invoke
File script1.js implements Sensor1 Web Thing and looks like this: // ... define a TD, produce an ExposedThing called sensor ...
sensor.setPropertyReadHandler("temperature", sameFancyFunctionToReadSensor1);
// Other Handlers/configurations Similarly, script2.js implements Sensor2. Now to implement the remainder sensor a dev could:
Either of the two options is perfectly reasonable. However, I see that option 1 has a greater advantage over the second: the same code will work even if Sensor1 or Sensor2 is not hosted in the same node. This means that the code that implements Sensor3 could be ported in different scenarios. For example, in applications where Sensor1 and Sensor 2 should be discovered at runtime and are not in the same node of Sensor 3. So a possible implementation of Sensor3 could be something like the following: function createSensor3( sensor1:ConsumedThing, sensor2:ConsumedThing){
// Define Sensor3 TD and produce sensor3 ExposedThing
sensor3.setPropertyReadHandler("value",async () => {
return await sensor1.readProperty("value") + await sensor2.readProperty("value") // or other super complex combination of the values
})
return sensor3;
} As mentioned, this exact same function could be used in an application that creates Sensor1 and Sensor2 or in another one that discovers Sensor1 and Sensor2 at runtime. This is the portability that I had in mind today. The good news is that we can still achieve this portability just with @zolkis's 1 and 2 points. However, there's a catch:
This implies that I still waste resources to create an HTTP (or other protocol) request, serialize it in a socket, wait for the response, decode it, and finally get the result. However, we could optimize it by defining a Finally, probably also this use case falls under "it is a nice addition but it adds too much complexity to the runtime". However, I found it as a basic functionality that would increase code reuse and encourage good design patterns inside WoT applications. |
I agree 1. and 2. seem sufficient and the convenience of 3. has been causing issues/confusion in the past. |
w.r.t. node-wot: Shall we revert the change of #328 to avoid issues with running samples out there (version 0.7.x). @egekorkan would this help you? |
Yes ! 😄
Me too. |
This seems hypothetical to me, since the script is running in a single runtime, it cannot locally access sensors on other nodes. The optimal solution for that seems to be a full-fledged Since we want to have that before exposing the Thing, we'd need to split the current That new function would resolve with a locally bound If the implementation cannot make the optimizations to have "local bindings", the new function should fail (all its purpose is ConsumedThing-like local interface). In that case the script would need to consume() the ExposedThing after it is exposed, or just work with the private functions. The nice thing about it is that it can be done on top of 1+2 and code won't break. Does that sound good, and is it worth doing it? |
API will change for v0.8.x anyway see discussion in eclipse-thingweb#333
API will change for v0.8.x anyway see discussion in eclipse-thingweb#333 Signed-off-by: reluc <relu.cri@gmail.com>
The new scripting API and the according node-wot implementation does not have |
Latest release does not allow
readOnly
properties to be updated within the thing itself, e.g.examples/scripts/counter.js
crashes on this line.The text was updated successfully, but these errors were encountered: