-
Notifications
You must be signed in to change notification settings - Fork 339
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
Change links
to forms
in API
#2806
Comments
Implementation proposals for the current open points:
Note the proposed solutions have as assumption that we cannot modify the current spec, |
I don't think we should assume that. If there are still things that can't be expressed in a W3C Thing Description then we need to flag that with the W3C, and in some cases it may be reasonable to add proprietary extensions for unsupported features, remove some features that can't be supported, or keep some features in the API but exclude them from the Thing Description if there's no way to describe them.
I'd really rather not do this as it creates a lot of redundancy and may result in clients opening a WebSocket per interaction affordance which is highly impractical (see #34). (If anything we'd like to go in the other direction and offer a single WebSocket endpoint for the gateway shared by multiple devices). I would prefer one the three possible options, in order:
Sorry, but this seems like a horrible hack. For properties I think it's already possible to define a top level form:
If we can't think of a way to implement the actions and events resources in a W3C compliant way, or a proposal to the W3C for how to make this possible in the specification, then we may just need to drop those features. They are really just convenience features which duplicate the individual action and event endpoints. This may be discussed further in #2807. |
Thank you for sharing the intended direction. I was not aware of how you would like to handle this issue so I had to make a hypothesis. I thought that chasing a moving target would slow the convergence process and WebThings would follow the TD 1.0. It might take some time before a feature would be included in TD 1.1 and also, TD 1.1 must be backward compatible. As you said we could use property extensions while waiting for the standardization to followup but this would possibly create fragmentation (what if W3C never includes the extensions? are those extensions functional to the communication between a consumer and a WebThing? if so not WebThings clients would not be interoperable). Having said that I am not against following this path, but we should take some precautions (that I think you are well aware of). About the proposal, as the hypothesis does not hold anymore, I think we could totally discard them. They were designed to strictly follow TD 1.0 so there's no point continuing the discussion on them. Instead, about your proposals, I agree with the suggested order of possible solutions although I might have some comments/questions about the first one (let's discuss them in the issue you opened).
It was 😄, but again I assumed that we would NEED those features and we cannot modify the spec. So it would have been odd to use |
Yes, sorry. Your solutions were completely reasonable within those constraints. I'm just conscious that whatever design decisions we make for the gateway's API will be probably be replicated in around 17 other WebThings implementations, so it needs to be something we're happy to live with long term, not just forcing W3C compliance for the sake of compliance. (No pressure 😉)
If we think we can get better solutions into 1.1 then I think it's reasonable to pursue that rather than limit ourselves to what's possible with 1.0. Where there are conflicts I think we'll need to take it on a case by case basis as to whether we try to improve the W3C spec, drop a feature from WebThings, or implement it in a different way.
We should try to find a solution to propose for TD 1.1, but if we can't then we may have to remove the actions resource and change the gateway front end to call individual action resources instead which shouldn't be too much of an issue. Currently the gateway front end fetches the events resource to get a log of all events, that could be changed to fetch individual event resources but that would require merging multiple lists and would still require a way to describe an event log. Let's discuss that in #2807. |
PRs in flight. Notice that the changes on the WebThings gateway depend on these two PRs, we have to wait until they are merged and published. |
@relu91 Thanks for sending these first PRs, very exciting! This code has changed a lot since I last worked on it and after I left Mozilla Mike replaced nanomsg with a new WebSocket IPC mechanism, which we'd wanted to do for ages but I'm not familiar with. I was never that familiar with the implementation of the add-ons system in the first place as I mainly worked on the web layer. I have to admit I wasn't expecting that changes would be necessary to the adapter IPC layer in order to change the format of Thing Descriptions exposed by the gateway's API, I had naively imagined the changes would be limited to the models and controllers of the web application. I clearly need to do some homework (including familiarising myself with TypeScript syntax!). In the meantime I wonder whether @tim-hellhake can help here since he is the new module owner of the add-ons system and should probably review this code. Now that I know the add-on IPC needs to change I'd like to better understand how this will impact add-ons. Will this break backwards compatibility for add-ons and mean add-on developers need to update their add-ons to use the new IPC? Or is this all handled within the node and python bindings provided by the gateway? I ask because I told people that add-ons wouldn't need to change and I'm concerned that I may have been mistaken. Also, with regards to branching I'm thinking of creating a v1.x branch in the gateway repo so that we can continue to release 1.x releases without worrying about breaking changes on master. @tim-hellhake Do you have any thoughts on how to handle this in the gateway-addon-node, gateway-addon-python and gateway-addon-ipc-schema repos? How can we make sure that the right branch is pulled into 1.x and master (2.x) builds? |
I see. I am new to the code base so I might as well touched the wrong points. My process was to start from the Lines 18 to 22 in 627c7c4
Those are used to model the final thing description instance and are later "serialized" in a ThingDescription here. All of these types are defined as JSON schema definitions in gateway-addon-ipc-schema and later built to TS types in gateway-addon-node. Al alternative approach (that I evaluated) would be to detach the Thing class from the schema definitions, but it would mean to have duplicated definitions. Not ideal in my opinion. I saw that the list of the addons is quite long and it is not easy to predict the impact of this change. We can't even rely on TS-type checks. I picked addon at random and, as described in the gateway-addon, it does not declare it as a dependency. However, it redefines some of its types... I think in practice this describes the worst of scenarios: The same I think applies to python-based addons. |
It depends.
I can't say for sure which is the best option because I don't know all the necessary changes.
Imagine the old spec is your transport protocol.
The current PR changes the API so the addons will definitely break.
I think we should pursue the same strategy in all addon repos.
The CI pipeline will publish the correct version of the |
You are right.
The As it will be a new major release I would rather break things and do it right instead of maintaining technical debts. |
@tim-hellhake wrote:
This makes sense, thanks for explaining. I share your instinct to choose option 3 to have a cleaner solution which can support all current and future W3C WoT Thing Description features. However, this may have significant implications because it could break many of the 100+ existing add-ons. Changing the API exposed by the gateway was supposed to be the easy part, as I thought that would only impact the gateway's own web interface. And whilst it sounds like that is technically possible, it isn't a very future proof solution because the same schemas and terminology are used all the way down the stack and it would make sense to change those to the W3C compliant versions. We already said that changing the API consumed by the thing-url-adapter would require further discussion as it means breaking compatibility with 16 web thing libraries. Therefore a change that could impact up to 100 add-ons definitely requires further discussion. If we think that option 1 is a reasonable first step, then I think we could push ahead with that now. It could definitely fix this current issue of simply renaming "links" to "forms", but once we start more significant changes to the API and potentially adding support for W3C features which aren't in the Mozilla spec I suspect that is going to unravel. If we want to go with option 3 then we need to hold a discussion with the community. @tim-hellhake @relu91 Are you both available to join the public monthly meeting on Thursday and explain these three options and their implications? The decision on which approach to choose may also depend on the timing of when we release these changes. If it coincides with a replacement base OS which ends up requiring re-architecting the add-ons system around containers of some kind, then changing the API surface wouldn't be such a big deal as we may break compatibility anyway. But I wouldn't want to block releasing the W3C compliance features on a new base OS and add-ons architecture and create a gigantic 2.0 release plan that takes years to finish. We need to figure out an incremental release plan which allow us to continue with regular releases. |
Hi all, A thought on migrating gateways to this compatibility-breaking 2.x update: If there is a common structure for branching as @tim-hellhake recommends then the 1.x gateway update code could check the installed add-ons to see if each has a new 2.x version which is available and inform the gateway user before updating, e.g. "You have 3 add-ons (list them) which will be incompatible with the new update, do you want to update?" If "yes" then the gateway would then update the gateway and all add-ons for which a compatible 2.x version exists. The update code would also ensure that add-ons are not updated to v2.x compatible version whilst the gateway is still 1.x. Would that fly? 🤔 |
@madb1lly I think we may want to do something along those lines regardless, so thank you for the suggestion. I know NextCloud does this kind of thing. When upgrading between versions it tells you which "apps" are compatible with the new version of the core and which will need to be disabled. @tim-hellhake @relu91 I just talked over this issue with a mutual friend and we concluded that the impact of option 3 may not actually be that bad. The only two instances we're aware of where add-ons manipulate the links array are:
We can update the thing-url-adapter ourselves and the UI links will stay as links so don't need migrating. With regards to other changes we want to make for W3C compliance:
All in all the impact actually seems minimal. What do you think? |
I'll be there 👍🏻
Well given this information I think we could really handle option 3.
Do you mean that the gateway should still load and TD with links instead of forms? but still, expose TDs with forms? I am asking because in the current changes that I have done I did not assume this backward compatibility. Of course, I can update it. |
With In theory we could do the same for links vs. forms but in practice it might be trickier because some links are going to stay as links. I've also just noticed another difference we'd need to handle in #2810. |
yeah, the translation of mediaType should be straightforward. I am a little bit worried about consistency from the add-on point of view. I mean in the new version we enforce link to be formed and only deprecate mediaType; either we choose to deprecate both or translate both. I know that for mediaType is just really a renaming operation, but still a little bit confusing. is it? |
The difference is we can't deprecate links, because we still need them in W3C compliant Thing Descriptions, e.g.
Not all links need changing into forms.
|
I see, I thought we could do a "scoped" deprecation only for links in affordances. Still, since we are already in breaking backward compatibility mode, I don't think that it would make so much difference to maintain |
I'll be there too.
Excellent.
I thought we don't need to break compatibility because we just add |
It's probably to be consistent with link elements in HTML... |
MicroBlocks has a WebThings library that I am pretty sure will need to be updated. For Wi-Fi devices such as ESP8266 and ESP32 and others, it produces a "native" web thing description (TD) per the current webthings.io/schemas. Currently my simple web doorbell TD looks like this (as loaded via "curl http://DEVICE_IP_ADDRESS"): And a simple "smart light" (LED on/off) looks like this: What should the JSON TD look like to be compatible with the W3C spec, which if I understand correctly, the gateway's updated thing-url-adapter will be looking for? If these TD's didn't change, would the WebThings Gateway of the future be able to discover and manage these devices or not? If not, I assume everyone who has developed webthings.io/schemas devices will need to abruptly update deployed devices, plus fix all examples and documentation to use the updated spec. |
@kgiori This issue is only about the Web Thing API exposed by the gateway (phase 1 of the W3C compliance plan), not the Thing Descriptions consumed by the thing-url-adapter, so web thing libraries won't need to be changed. The backwards incompatibility at the add-ons IPC layer we're talking about will be handled automatically by the thing-url-adapter, which we will update as necessary (along with the other two add-ons we believe will be effected). The Thing Descriptions exposed by native web things (like the ones created with the MicroBlocks web thing library) won't need to change. Once we get to phases 2 & 3 of the W3C compliance plan (changing the thing-url-adapter and web thing libraries to use W3C compliant Thing Descriptions), we're looking at ways of supporting both the legacy Web Thing API and W3C compliant API simultaneously (e.g. with two separate adapters), so that web things have plenty of time to change over. Don't Panic ;) |
Sorry, I got caught up in a meeting this Friday and I didn't have time to polish and submit the PR. Now is on 😄 .
+1 for the legacy adapter it will decrease the "stress" on the WebThingsIO framework libraries.
With the current changes the two TDs will not change too much: /* No change here we have still to understand how to translate root links
* to forms.
*/
{
"title": "doorbell",
"@context": "https://webthings.io/schemas/",
"@type": [
"PushButton"
],
"links": [
{
"rel": "events",
"href": "/events"
},
{
"rel": "properties",
"href": "/properties"
}
],
"properties": {},
"events": {
"pressed": {
"description": "MicroBlocks event",
"@type": "PressedEvent"
// <-- Here we need a form. Still an open issue as said in the comment above
}
}
} {
"title": "Hello LED",
"@context": "https://webthings.io/schemas/",
"@type": [
"Light"
],
"links": [
{
"rel": "events",
"href": "/events"
},
{
"rel": "properties",
"href": "/properties"
}
],
"properties": {
"on": {
"forms": [ // <-- changed to forms
{
"href": "/properties/on"
}
],
"title": "On",
"type": "boolean",
"@type": "OnOffProperty",
"readOnly": false
}
},
"events": {}
} |
An update on the open questions...
My conclusion from w3c/wot-thing-description#1070 is that this can be a form, and that we can simply omit the
My current thinking is that:
|
@tim-hellhake about this I recently landed a PR that introduces the typescript definitions based on the W3C JSON schema. It is still not published yet, but maybe we could re-use it in our IPC types in the future. |
See further discussion on this topic in w3c/wot-thing-description#878 My preference is still to provide a single top level |
I've just been testing #2811 and it's looking good. My understanding of what is still missing:
Since #2830 is now blocking this issue, I'm going to start looking into implementing Server-Sent Events support. |
I agree with the direction described in the comment. Thanks to the fact that we already have a Rest API in place, TD consumers will still be able to interact with the Web Thing. Plus the TD will be valid according to the schema. What might be a little bit annoying is how we describe the situation in the TD spec:
Option 2 would be optimal for our purposes because it will make our TD valid even on the assertion level.
Oh sorry, I miss that, I'll push an update soon. |
fixes WebThingsIO#2806 Co-authored-by: Ben Francis <[email protected]>
fixes WebThingsIO#2806 Co-authored-by: Ben Francis <[email protected]>
fixes WebThingsIO#2806 Co-authored-by: Ben Francis <[email protected]>
Blocks: #2802
The gateway's API currently exposes
links
for each interaction affordance (property, action and event) which provide API endpoints for getting/setting properties, invoking actions etc. E.g.The W3C WoT Thing Description specification specifies the use of
forms
rather thanlinks
for many of these use cases. E.g.I still do not like the forms approach of the W3C spec (see w3c/wot-thing-description#85 and w3c/wot-thing-description#88 for some previous discussions), but given for simple use cases the two are functionally equivalent, let's pick our battles and make this conformant with the W3C spec.
This includes:
links
toforms
where appropriatemediaType
tocontentType
where usedrel
(e.g.rel: "property"
) from links which become formsop
member of a form where needed (this is not needed in all instances due to default value definitions)This excludes:
response
metadata). This will be handled separately.href
endpoint for these interaction affordances and continue to follow the conventions of the Web Thing API.Open questions:
could become:
but this is not technically W3C compliant as the spec says:
and its not clear how to represent WebSocket API message types which are pushed from the client to the server such as
propertyStatus
,actionStatus
andevent
.The top level properties endpoint could become:
but there are currently no values of
op
which map well onto "get all recent events" or "invoke an action whose name is provided in the request payload". If we try to remove therel
from the actions and events endpoints, it will therefore no longer be possible to distinguish what they are for.The text was updated successfully, but these errors were encountered: