search

w3c / wot-discovery

Repository for WoT discovery discussion
https://w3c.github.io/wot-discovery/
Other
20 stars 17 forks source link

Review Comments from Ege Korkan #178

Closed egekorkan closed 2 years ago

egekorkan commented 3 years ago

This issue will be a quite long one and maybe too late for the specification at this point but better late than never.

I have started thinking about this after getting triggered by the issue 1143 in the TD spec. I have discussed with @sebastiankb , @danielpeintner, @wiresio and @Fady222 but the thoughts are somewhat mine.

I have a couple of concerns with some aspects of the Discovery specification that I am going to write about in this one issue but after a discussion, they can be put into different issues:

  1. TD of the TDD
  2. HTTP focus
  3. Scripting API compatibility

TD of the TDD

I find the TD of the TDD to be rather difficult to understand. This is coming from me who knows the TD specification well enough after working with it for so long, it will be worse for a newcomer. In order to understand this TD, one needs to understand the details of the TD spec (more the forms chapter). Even then, it just does not look like any other TD I have seen, which means all the TDs that had been submitted in the TD 1.0 implementation report generation and all the TDs that 100+ students of mine wrote so far. I think this view is shared by @benfrancis with his comment at https://github.com/w3c/wot-thing-description/issues/1143#issuecomment-843248219. We are trying to shape the TD so much that it can finally describe what the Discovery REST API is. I have no criticism on this API, I actually find it pretty good. However, trying to describe this with a TD is like a stepping stone to describe the GitHub REST API with a TD. It might be possible but TD is not designed for this. This creates weird structures that are difficult to understand for someone (or a program/consumer) who understands TDs and also difficult for someone who just wants to use the Discovery API as a normal REST API.

An Open API document would be better suited for this kind of APIs, like what LinkSmart has. TD Playground has a TD to OpenAPI converter but it will fail (silently or not) at such a complex TD.

It is important to note that the need to have a TD for the TDD is not listed in the requirements. There is the requirement called Be compatible with discovery support in the WoT Scripting API. but I think that is not what is meant and I will talk about this in the corresponding point below. Why are we trying to do this, just because we can?

Furthermore, this need to have a TD for the TDD that can describe the whole API is creating features in the TD spec that are needed only by the discovery API which is a service and not a Thing. What if this results in people creating Thing implementations (like a lamp) that have these features. I remember some newcomers creating TDs that had action affordances with uriVariables in them simply because we had an innocent and informative example about uriVariables that had an action affordance. In summary, when we put something in the TD spec, nobody from the outside will know that it came from the discovery specification and assume it is the supported and well-understood way.

Proposed solution path:

  1. Either removing the TD of the TDD in order to not confuse people anymore or simplifying such that it does not describe the entire API with all the different response types etc but reducing it to something mostly useful for registering TDs and maybe searching for some. A fully conformant implementation will probably read the specification anyways and not base their implementation purely on the TD. Is it realistic that one can write a fully conformant discovery consumer implementation without ever reading the discovery spec but only reading the TD spec?
  2. Having an OpenAPI document for the API. Open API is not a standard but it can be put as a helper document that people can use to understand the API better, like how we put a JSON Schema in the TD spec so that people can validate it easier. What makes this spec normative is the text in the spec anyway.

Some other points around this topic

  1. For me, TDD is not a Thing. Thus, it SHOULD NOT have a TD. Maybe this is a philosophical direction so it can be ignored but I would stand firmly with this opinion.
  2. TDD API is mostly static. There are some parts that can change based on implementations but there are assertive sentences like The API MUST allow registration of a TD object passed as request body. so it is not that I can have a TDD TD that does not have createTD affordance. TDs work better when there is a variety of devices that need to be described. So once my TDD TD consumer knows that this is a TDD and it will not necessarily parse its TD to create requests.
  3. This TD is not core profile (or whatever it is called now) compliant. It is not even compliant with my brain's parsing capabilities and the profiles are about reducing parsing and consuming capabilities. Of course, I am aware that this TD is not claimed to be core profile compliant but when we are talking about easing the consumption of TDs to enable interoperability, this is a step in the opposite direction.
  4. If having a TD for TDD becomes a thing (small pun in the middle of this long issue), i.e. widely used by everyone, then each TD consumer will need to have a check by using @type. If it is a TDD TD, they will need to do some special handling of certain actions etc. This will cause more annoyance than it helps by unifying interfaces.
  5. I can see some use to having a TD when I force myself enough:
    • There is a possibility to have TDDs that can do only a small amount of stuff, like when the database of the TDD is static and it does not have a createTD method. This can be applied for other affordances, like not being able to update a TD because this is another special type of TDD. In short, if the spec is more loose, it starts making more sense but I don't think that the spec should be more loose.
    • We really have other protocols. Since it is not the case at the moment, let's remove the TD and add it later when we have other protocols and that the API is protocol independent (without breaking changes). If the TD of the TDD is really meant to be protocol independent already, i.e. I remove the forms and everything still makes sense for a protocol like MQTT and there can be an implementation as well, I volunteer to scribe all the meetings for two weeks.

HTTP Focus

I think that the specification needs to be better categorized so that other protocols can be used in the future. I totally support the fact that HTTP will be the first protocol to support but it is not clear from the spec how other protocols will work. The different affordances should be abstracted from the HTTP protocol and made clear what are their inputs and outputs. The spec does this 90% but it needs improvement. What I mean is that there should be an explanation for what a createTD affordance needs (a TD input) and what it will deliver (an id). Then, in an HTTP binding of this affordance, it can be specified how this should be done in the case of HTTP, which is what is done now.

This way, the information that is relevant for the Consumer is clear and is not lost in the protocol information. This would mean that spec should have first an abstract section and then a protocol binding for HTTP. I know that this sounds like a big change but I don't think that it is that bad. In the screenshot below, the part before the bullet points is the abstract section and the bullet points are the protocol-specific part:

image

Proposed Solution:

Relevant Issues:

Scripting API Compatibility

Scripting API is not a normative spec, but for me, it always works wonderfully as a sanity check whether we are respecting our protocol independence motivation, i.e. if we add a feature to TD, how is this information separated for application-logic-specific information and protocol-specific information.

So, if we have the goal of having a TD for the TDD that can simply be used by a Scripting API implementation like it is a normal TD, this TD of the TDD simply does not work. The UUID of the registered TD is in the header, thus the implementation cannot simply use a normal invokeAction method and get this information from the header. It will need to know that this is a TDD and do some special handling that is only valid for HTTP. Thus, if I were to implement the Discovery API in a Scripting API spec implementation, I would prefer to have methods like createTD, updateTD that have their corresponding implementations per protocol and not something like invokeAction(createTD, myTD). So when I would do this implementation, I would not use the TD of the TDD but simply read the spec and implement it from scratch for each protocol. This further motivates my point to have no TD for the TDD.

An alternative to having createTD methods would be to differentiate the Consumed Thing object into normal Things and Directories such that the invokeAction operation behaves differently for the two. This would again cause more annoyance than it helps by unifying interfaces. Now, even the human developer needs to pay attention that abc.invokeAction can be different than def.invokeAction since abc is a normal Thing and def is a TDD.

TLDR

benfrancis commented 3 years ago

@egekorkan wrote:

I think this view is shared by @benfrancis with his comment at w3c/wot-thing-description#1143 (comment). We are trying to shape the TD so much that it can finally describe what the Discovery REST API is. I have no criticism on this API, I actually find it pretty good. However, trying to describe this with a TD is like a stepping stone to describe the GitHub REST API with a TD. It might be possible but TD is not designed for this. This creates weird structures that are difficult to understand for someone (or a program/consumer) who understands TDs and also difficult for someone who just wants to use the Discovery API as a normal REST API.

For me, TDD is not a Thing. Thus, it SHOULD NOT have a TD.

I fully agree, see #172.

Having a TD for a directory seemed like a good idea, I even suggested it myself. But I had imagined something much simpler which just provided a list of links to TDs or a single link to a resource which provides a list of TDs (see https://github.com/WebThingsIO/api/issues/91), not describing a complex web service using Forms in the Thing Description.

The idea of providing a list of Links to Things was not pursued because it would make the TD too dynamic, which I agree with. But another alternative could be just to link to the top level endpoint of the Directory Service API. See my proposal in https://github.com/w3c/wot-discovery/issues/172#issuecomment-844022530.

We currently have more agreement on what the REST API should look like than we do on how to describe it in a TD. The areas of the API on which we have yet reached a consensus (e.g. having a single event or multiple events and having update and partial update combined or separate) would be simplified by just designing a good HTTP API and not trying to design something protocol agnostic which can be described in a TD and consumed with the WoT Scripting API.

An Open API document would be better suited for this kind of APIs

Having an OpenAPI document for the API. Open API is not a standard but it can be put as a helper document that people can use to understand the API better, like how we put a JSON Schema in the TD spec so that people can validate it easier. What makes this spec normative is the text in the spec anyway.

Agree.

Either removing the TD of the TDD in order to not confuse people anymore or simplifying such that it does not describe the entire API with all the different response types etc

let's remove the TD and add it later when we have other protocols and that the API is protocol independent

Agree.

The different affordances should be abstracted from the HTTP protocol and made clear what are their inputs and outputs. What I mean is that there should be an explanation for what a createTD affordance needs (a TD input) and what it will deliver (an id). Then, in an HTTP binding of this affordance, it can be specified how this should be done in the case of HTTP, which is what is done now.

I'm not opposed to this, but I don't think it's really necessary. Let's just concentrate on designing a good HTTP API first, and worry about abstracting later if it's necessary. I see no reason someone couldn't just create a separate or extension specification in the future which defines a directory API using another protocol if they want to, there's really no reason for it to be described in a protocol agnostic way in a Thing Description.

I have submitted a PR https://github.com/w3c/wot-discovery/pull/179 with a proposal for a much simpler TD for a Directory which is just used for discovering its top level API endpoint, rather than trying to describe the whole web service using Forms.

egekorkan commented 3 years ago

@benfrancis wrote:

I fully agree, see #172.

I should have seen this issue first, sorry about that.

Regarding the protocol abstraction, what I mean is that if I am creating a convenience library that acts as a client to the directory, I would like to have some guidance on what should be left to the users and what should be handled internally. The programmer wouldn't even need to know that this is an HTTP client. Each library can document it on their own but there are some critical information that are always valid, no matter the protocol or library. It would be better to see this.

This can be done later once the API is fixed for sure since it is only about extracting information from something already existing.

mmccool commented 3 years ago

Ege, thanks for your input. Let me address these using the numbered categories in your opening statement. I'll start with 2 because it influences 1.

Point 2 addresses the current overreliance on HTTP. The original intention was that we would focus on HTTP for this version only to limit scope, but would design things so that the API would be easy to extend to other protocols in the future. Unfortunately it seems some HTTP-specific things have crept into the spec. Actually, I missed the id-in-the-header thing and agree, that is not compatible with extension to other protocols. I consider that a bug and an issue should be raised to correct. Another thing still underheavy discussion is pagination/chunking/blocking. I expect however that no matter how good a job we try to do in this respect, trying to extend the API to other protocols will uncover some issues that will probably require extensions to the API in a future version. But I would at least like to aim at the goal of those extensions being backward compatible with this version. I should mention some are unavoidable, in particular, we want to be compliant with the SPARQL spec for that query method and it specs a specific HTTP API.

So this gets to point 1. If we DO want to support multiple protocols, OpenAPI will not get us there, being HTTP specific (although there are a few extensions to support CoAP, these are not complete). I should also say some of the things we are struggling with in the TD spec, like dynamic resources, are also troublesome in OpenAPI. The problem is we also have no direct influence over the OpenAPI spec so can't address these problems. Of course we could use a full prose specification, but this will be less precise.

There ARE pragmatic reasons to define the TDD API in a TD:

  1. Introductions sometimes return ONLY a URL, with no indication whether it points at a Thing (Peer-to-Peer discovery) or a Directory. Right now dereferencing such a URL always gives you a TD, then you look at the @type to figure out which you are dealing with.
  2. The discovery filters in the Scripting API are going to be fairly limited, just keywords. If you want to use the full power of even JSONPath, let alone SPARQL, you will need to access the directory API directly. If the Directory is a Thing, and node-wot can consume its TD, then you could use the Scripting API to do more sophisticated queries, including templates and semantic search.

Of course there might be other ways to do these. For example, for 2, you could use an HTTP library to access the Directory from a script. But, this would be (a) protocol-specific (b) not guaranteed to be available in all contexts (c) more complicated.

I also want to address the following two "philosophical points":

  1. The TD for the TDD is more complicated than others we have looked at. I think this is an artifact of us just not trying very hard to model more sophisticated devices. Even my attempt to model an IP camera resulted in a much longer TD than any others. I also just installed a WeatherFlow Tempest weather station. This has a VERY sophisticated local HTTP-based API (and also a service that stores historical data, gives weather predictions, etc). I'm sure modelling it would result in a very complex TD. Likewise my Victron solar energy storage system has sophisticated APIs, can store gigabytes of historical data, allow queries, setting up of rules, etc. etc.
  2. TDs should be used for Things, not Services. I disagree strongly, and several of the use cases I have submitted are about supporting services. In fact most of the use cases Intel cares about are (edge) services, like analytics, that are most interesting when combined in mashups with sensors and actuators. Allowing at least Edge Services to be modelled as TDs makes it much easier to build mashups. Two examples I can think of are a person-detection neural net service (running on the edge, but maybe not) being used as part of a security system, or a weather prediction service (actually supported as part of the Tempest device, above). If you wanted to be strict about this, the boundary is fuzzy. Is an IP camera with recording capability a service? Is a gateway? If we try to be overly strict about this we will be throwing out important use cases. It's true that because a directory is a database and can store data posted to it it's a little different than other Things we have looked at (so far), for example. But it's ALSO true that I can imagine "regular" Things (like smart plugs with timers; my Sonoff smart plugs support an arbitrary number of creatable timers, and in an API these would probably be dynamic resources) where I might want to do something like that. Also it turns out state is needed even for trivial orchestration tasks like having a trigger with hysteresis so a "state" service (as a Thing or as part of an API) WILL come up in applications (I have a specific use case where I want to turn a device on when another goes above threshold, and off when it goes below. I want to avoid sending multiple on and off messages. It turns out this is super annoying to implement without state, eg using only stateless FaaS trigger rules).

Again... I think we just have not gone far enough in trying to model sophisticated devices, including many available commercially and in important use cases. The reason the Directory is raising issues is not because its doing things that I would not imagine a "normal" Thing doing, it's just that it is doing them FIRST.

That said, I do think we need to address specific issues you raised in 2 and 3. We should reduce the dependence on HTTP specifics as much as possible, and flag and fix such dependencies. We should make sure the TDD is usable, as a Thing, from the Scripting API (at least for basic functionality).

mmccool commented 3 years ago

Discussion in meeting:

mmccool commented 3 years ago

We've discussed this at length now and (probably) come to a consensus/conclusion on most of the issues raised. I would like to propose closing (please confirm @egekorkan) but then reopen any specific single issues that still need to be addressed by the next WD update (Sept 30) or by CR (with appropriate labels).

farshidtz commented 2 years ago

From Discovery call: