Refactor Directory Service API TD #184

benfrancis · 2021-05-24T18:25:36Z

OK, here is my last attempt to re-factor the Thing Description to something which we may be able to reach a consensus on, before taking the nuclear option of #179

This PR is a combination of #158 #159, #160 and #161 based on the discussion in #133, but with compromises based on feedback on the original PRs and conversations in the last WoT Discovery call.

(Sorry this is one big PR but some people were asking me to combine PRs and some people were asking me to split them up further and this just seemed the easiest way to see all the related changes together.)

Below is a summary of the changes:

@type - renamed DirectoryDescription to ThingDirectory to be more consistent with Thing and ThingLink as discussed in Refactoring TDD Thing Description #133 and Define the WoT-Directory type somewhere (eg in new version of context, in 1.1) #43
properties
- things - Renamed from retrieveTDs so that the property name is a noun.
actions
- createThing - Renamed from createTD
- retrieveThing - Replaces retrieveTD property since it's a verb
- updateThing - Renamed from updateTD
- partialUpdateThing - Renamed from updatePartialTD
- deleteThing - Renamed from deleteTD
- searchJSONPath - Now an action rather than a property since it is a verb
- searchXPath - Now an action rather than a property since it is a verb
- searchSPARQL - Now an action rather than a property since it is a verb
events - split out into three separate events, but keeping type as a query string in the URL
- thingCreated
- thingUpdated
- thingDeleted

Notes:

The interaction affordances for retrieving individual things and searching for things are now actions rather than properties as @mmccool suggested and @relu91 mentioned would be consistent with an OOP type programming model
Pagination URL variables are still part of the things property since they operate on the collection of things rather than an individual thing. @farshidtz alluded to the fact that making pagination an action would be silly.
I've separated out the events into three separate event affordances but kept the type query string in the URL, since @farshidtz said he'd be OK with splitting the event affordance as long as the type query string was still in the URL
- Note that the subscribeallevents feature will be needed in the Thing Description specification in order to describe how to subscribe to all events at once
I've kept updateThing and partialUpdateThing separate interactions to alleviate concerns about including both a PUT and PATCH form in the same interaction affordance, and so that Add op to support PATCH wot-thing-description#1143 is not necessary in the Thing Description specification
I've removed the semantic annotations for all the interaction affordances, data schemas and URI variables since they are largely redundant if the interaction affordance names and URL structure are normatively defined in the specification as @mmccool thinks they should be
The TD still tries to describe the Directory Service API using Forms, but hopefully those forms are now slightly (but not much) simpler by splitting some things out (which is @egekorkan's main concern)

Things I like about this compromise:

Property names are now nouns
Action names are now verbs
Each event type now has its own event affordance

Things I don't like about this compromise:

Retrieval and search features are actions. In my view these features should be part of the things property because they each just provide a different filtered view of the data in the things property and don't change state in any way. It feels inconsistent to have retrieving the whole list of things and paginating that list as a property, whilst retrieving a single thing or searching the list by JSONPath/XPath/SPARQL is an action.
The names of interaction affordances, URLs and URL variables are hard coded in the specification. In my opinion this is too prescriptive and developers should have the freedom to choose their own affordance names and URL structure, by using semantic annotations to identify which is which.
Event URLs use query strings instead of paths to identify different events. I don't really understand the reason why this is better.

I still don't think Thing Descriptions are well suited to describing a web service like the Directory Service API (as opposed to the capabilities of physical devices). I also suspect that #179 could actually be a quicker route to workable specification within the time-frame of the current charter. However, please let me know whether you think this could be a workable compromise.

Preview | Diff

relu91 · 2021-05-25T07:22:30Z

directory.td.json

-        "updateTD": {
-            "description": "Update a Thing Description",
+        "retrieveThing": {
+            "description": "Retrieve an individual Thing Description by its ID",


I think this action is missing the output parameter. Like the events we could just say:

output : { type:"object", description: "The retrieved Thing Description" }

The full JSON schema of the TD would be better but it super verbose. It is something that we need to improve in the TD spec (e.g., use models to import json schema type definitions). Besides, the content type gives this information so it might be ok to simplify right now.

I thought this information was provided by the contentType, but I have added the output data schema as well for good measure.

relu91

I think the proposal looks great. It is the best description we can achieve with the current situation. What I like besides @benfrancis points is:

low out-of-band information: a generic wot client would just need to know that things property is the collection and it has actions to manipulate/search for things.
It is less REST-oriented and it shows implementers how one can map GET requests to actions (nothing wrong with that IMHO).
Most of the times forms can be easy selected (not too much ambiguity here)

I know it is not perfect we have still open points:

the createThing action may return an id that might be used to handle the created resource. I consider this another minor out-of-band information cause it can be managed at the application level
The id is returned in the HTTP header, how can we express that the output is in the header and not in the body?

Besides, those concerns I would say that this proposal is an improvement of the current situation and I approve it. There's just one minor change that I pointed in the inline comment, after that considered the PR approved from my side. 👍🏻 Thanks, @benfrancis for your efforts.

relu91 · 2021-05-25T07:38:11Z

The names of interaction affordances, URLs and URL variables are hard coded in the specification. In my opinion this is too prescriptive and developers should have the freedom to choose their own affordance names and URL structure, by using semantic annotations to identify which is which.

In my understanding, this would be solved once we convert the TD to a Thing Model, right @mmccool? at least in regards to the URLs.

farshidtz · 2021-05-25T07:45:31Z

Thanks for the proposals. Here is my general feedback. I will look deeper into the TD and add inline comments.

@type - renamed DirectoryDescription to ThingDirectory to be more consistent with Thing and ThingLink as discussed in #133 and #43

This will cause a git conflict. It was already merged into the refactor branch; see #161. I have cherry picked your commit in #181.

The interaction affordances for retrieving individual things and searching for things are now actions rather than properties as @mmccool suggested and @relu91 mentioned would be consistent with an OOP type programming model

Things I don't like about this compromise:

Retrieval and search features are actions. In my view these features should be part of the things property because they each just provide a different filtered view of the data in the things property and don't change state in any way. It feels inconsistent to have retrieving the whole list of things and paginating that list as a property, whilst retrieving a single thing or searching the list by JSONPath/XPath/SPARQL is an action.

As discussed in the call, I think it makes no sense to model retrieval of one thing as action, but retrieval of multiple/all things as property. We need to be consistent on how we model the interactions. Either we consider the "change in state", in which case retrieval and search would be properties, or we consider the "verbs" and model everything as actions. In the latter case, we should set safe and idempotent to true. See https://w3c.github.io/wot-thing-description/#actionaffordance

events - split out into three separate events, but keeping type as a query string in the URL

thingCreated

thingUpdated

thingDeleted

Things I don't like about this compromise:
3. Event URLs use query strings instead of paths to identify different events. I don't really understand the reason why this is better.

I've mentioned this before: the API spec should "specify" a well designed API. Not the other way around. You are breaking the events API into three affordances and things start to look strange as a consequence. Looking at it now, I think splitting makes the TD much more verbose as you have to redefine everything that is technically identical. Note that you have missed the full query parameter (feel free to choose a better name). Also, you are changing the type values, which is good, but we need to remember and fix it in the text and examples. I think this change should have stayed in PR #159 (not sure why you closed that in favor of mixing it with others).

createThing - Renamed from createTD

There was a consensus to also split this into createThing and createAnonymousThing. See
#160 (comment)

farshidtz · 2021-05-25T07:57:37Z

directory.td.json

+                    "href": "/things",
+                    "htv:methodName": "GET",
+                    "response": {
+                        "description": "Success response",
+                        "htv:statusCodeValue": 200,
+                        "contentType": "application/td+json"


contentType should be "application/ld+json".

relu91 · 2021-05-25T10:07:23Z

As discussed in the call, I think it makes no sense to model retrieval of one thing as action, but retrieval of multiple/all things as property. We need to be consistent on how we model the interactions. Either we consider the "change in state", in which case retrieval and search would be properties, or we consider the "verbs" and model everything as actions.

The thing is that a property does not only convey an "operation" (i.e., retrieve all thing description) but also a semantic relation between resources. It is similar to a link if you think about it. So I think it would capture better the fact that a Thing Description Directory contains a list of Thing Descriptions. In this sense we could even improve the things property description using a semantic type:

{
things: {
  "@type" : "collection",
  type: "array",
  items: {
     type: "object"
     "@type": "item",
     description: "A stored Thing Description"
   }
  // etc...
}
}

With action, we would lose this meta-information about the relation between the collection and its container. Do you agree with this?

I do not see any "consistency" problem here, is a pattern well known in every programming language. However, I can see that a REST-API or GraphQL API is something less common.

farshidtz · 2021-05-25T12:51:55Z

@relu91, I don't see the analogy to programming languages. How are get(id)->TD, getMany(start, end)->[]TD, getAll()->[]TD, and search(query)->any different from each other? Why can't all be properties?

The design should be based on the TD spec, otherwise, it will become highly opinionated and subject to change for many years to come depending on who joins the TF.

relu91 · 2021-05-25T16:18:03Z

The design should be based on the TD spec, otherwise, it will become highly opinionated and subject to change for many years to come depending on who joins the TF.

I agree, but since it seems to me that we do not mutually understand each other... That's maybe because there is a degree of interpretation based on biases/expertise of the specification documents. If I read this:

A Property is an Interaction Affordance that exposes the state of the Thing. The state exposed by a Property MUST be retrievable (readable). Optionally, the state exposed by a Property MAY be updated (writeable). Things MAY choose to make Properties observable by pushing the new state after a change (cf. Observing Resources [RFC7641]). Write-only state should be updated through an Action.

and I have to design a Thing Description Directory. I think that the state of a TDD is the list of the contained TD. So yes it seems that the proposal is actually based on the spec.

Why can't all be properties?

Again in my humble opinion, it is not what the spec is saying. get* and search affordances are not a state, are operations that you would like to do on the state. I mean, nothing wrong to have getAll()->[]TD but it would be redundant cause you can do the same operation thanks to the WoT interface. In this sense, a readproperty is the same operation as your getAll().

About the programming language metaphor, I am referring to the following:

Usually, this abstract class diagram (it is a super simplified view of what you are building in other PRs) is coded in an OOP language as follows:

public class ThingDescription {}

public class ThingDirectory
{
	private List<ThingDescription> _tds = new List<ThingDescription>();

    public List<ThingDescription>.Enumerator  things // notice in c# those are called Properties
    {
        get => this._tds.GetEnumerator();
    } 
	
	public void createThingDescription(ThingDescription td){
	}
	
	/* all the other functions */
}

I am not saying that we should follow blinding this design, what I am saying is that the TD spec is pretty similar to that rationale.

farshidtz · 2021-05-25T16:52:17Z

Thanks for the explanation.

So querying all items from the database gives the state of TDD (assuming you meant the state of TDD.things), and querying just one item is an operation on the state? As if, everything is first queried to get the state and then one item is selected from it? This sounds like getting one item is more computationally expensive than getting all.

Even if considering all TDs as the state, one TD is just a subset of that state. There is no action / actuation involved. I could live with search as an action on the state.

Anyway, it doesn't matter what is placed where as long as the underlying API stays RESTful. I guess most consumers will get confused for a while and find the affordances eventually, unless we make them all actions and use the well understood verbs.

egekorkan · 2021-05-30T20:32:04Z

I also like this direction. Regarding property vs action debate, here is my comment:

I think that the main confusion is due to the use of the affordance names. In my opinion, there is a single affordance when we are talking about getting the TDs somehow (retrieve, all, search etc.). That is simply called TDs to me and there are simply different ways to obtain it. Even though it would cause a super complicated affordance, theoretically there should be a single TDs affordance with different forms or inputs or uriVariables for different ways to get what you want. If we had a series of historical temperature values, getting different subsets of this is still the getting historical temperature values, thus a single property.

Regarding the search, in Philips Hue Hub there is way to search for new lights using its HTTP API. There, the hub physically starts searching for a new light via Zigbee. That is for me clearly an action since the Hub does something. Here, we have mostly a query that is processed and replied with a value, thus to me this is still a property.

Since what I wrote above will make a weird/complicated TD, the current way is OK but not perfect.

farshidtz · 2021-05-31T10:31:13Z

In my opinion, there is a single affordance when we are talking about getting the TDs somehow (retrieve, all, search etc.). That is simply called TDs to me and there are simply different ways to obtain it. Even though it would cause a super complicated affordance, theoretically there should be a single TDs affordance with different forms or inputs or uriVariables for different ways to get what you want. If we had a series of historical temperature values, getting different subsets of this is still the getting historical temperature values, thus a single property.

@benfrancis already had a proposal for that. See #158 and the combined property: https://github.com/benfrancis/wot-discovery/blob/b075a81bcdfef34a2316ccc9509fe557140d9ce8/directory.td.json#L202

benfrancis · 2021-05-31T13:55:42Z

@farshidtz wrote:

we should set safe and idempotent to true.

OK, done.

I think this change should have stayed in PR #159 (not sure why you closed that in favor of mixing it with others).

Because you said "I propose doing all the renamings and semantic annotations in a single PR... because they don't make sense independently." #158 (comment)

I've mentioned this before: the API spec should "specify" a well designed API. Not the other way around. You are breaking the events API into three affordances

Again, it seems clear to me that these events are three different events with different meanings and you're only combining into a single event affordance to work around a limitation of Thing Descriptions, which would be solved in w3c/wot-thing-description#1082

There was a consensus to also split this into createThing and createAnonymousThing. See
#160 (comment)

I really don't think this is necessary, but please feel free to submit a follow-up PR for that as proposed in the comment.

farshidtz · 2021-05-31T17:07:09Z

#188 took parts of this PR and addressed few other open issues.

Remaining things to extract from this PR:

rename retrieveThings to things
move retrieveThing and search properties to actions (idempotent and safe)
split the events and rename event types but add back the full uriVariable (rename to diff)

I will add two more PRs to address the above as well as to update the relevant parts of the text.

mmccool · 2021-06-07T14:38:18Z

All changes have essentially been moved to other PRs (and merged, with updates). Will also close #133 since has been dealt with in other PRs. Closing without merging.

benfrancis added 7 commits April 22, 2021 19:04

Combine properties into a single things property

b075a81

Rename DirectoryDescription to ThingDirectory

bcf66a5

Rename actions to match properties and events

7c092a3

Address review comments

de43a89

Re-separate update and partial update

8157a1b

Split registration event into multiple events

9012314

Add compromises

a4a0ae7

This was referenced May 24, 2021

Combine properties into a single things property #158

Closed

Split registration event into multiple events #159

Closed

Rename actions to match properties and events #160

Closed

relu91 reviewed May 25, 2021

View reviewed changes

relu91 requested changes May 25, 2021

View reviewed changes

farshidtz reviewed May 25, 2021

View reviewed changes

benfrancis added 2 commits May 31, 2021 14:30

Add output data schema to retrieveThing action

a4294ab

Make read actions safe and idempotent

4dbbe47

farshidtz mentioned this pull request May 31, 2021

Refactor Directory Service API TD (td->thing, split anonymous create action) #188

Merged

This was referenced Jun 1, 2021

Refactor Directory Service API TD (retrieve one and search as actions, listing as things property) #191

Merged

Refactor Directory Service API TD (split events) #196

Merged

Improving event type values in the Notification API #197

Closed

mmccool closed this Jun 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Directory Service API TD #184

Refactor Directory Service API TD #184

benfrancis commented May 24, 2021 •

edited by pr-preview bot

Loading

relu91 May 25, 2021

benfrancis May 31, 2021

relu91 left a comment •

edited

Loading

relu91 commented May 25, 2021

farshidtz commented May 25, 2021 •

edited

Loading

farshidtz May 25, 2021

relu91 commented May 25, 2021

farshidtz commented May 25, 2021 •

edited

Loading

relu91 commented May 25, 2021 •

edited

Loading

farshidtz commented May 25, 2021 •

edited

Loading

egekorkan commented May 30, 2021

farshidtz commented May 31, 2021

benfrancis commented May 31, 2021 •

edited

Loading

farshidtz commented May 31, 2021 •

edited

Loading

mmccool commented Jun 7, 2021

Refactor Directory Service API TD #184

Refactor Directory Service API TD #184

Conversation

benfrancis commented May 24, 2021 • edited by pr-preview bot Loading

relu91 May 25, 2021

Choose a reason for hiding this comment

benfrancis May 31, 2021

Choose a reason for hiding this comment

relu91 left a comment • edited Loading

Choose a reason for hiding this comment

relu91 commented May 25, 2021

farshidtz commented May 25, 2021 • edited Loading

farshidtz May 25, 2021

Choose a reason for hiding this comment

relu91 commented May 25, 2021

farshidtz commented May 25, 2021 • edited Loading

relu91 commented May 25, 2021 • edited Loading

farshidtz commented May 25, 2021 • edited Loading

egekorkan commented May 30, 2021

farshidtz commented May 31, 2021

benfrancis commented May 31, 2021 • edited Loading

farshidtz commented May 31, 2021 • edited Loading

mmccool commented Jun 7, 2021

benfrancis commented May 24, 2021 •

edited by pr-preview bot

Loading

relu91 left a comment •

edited

Loading

farshidtz commented May 25, 2021 •

edited

Loading

farshidtz commented May 25, 2021 •

edited

Loading

relu91 commented May 25, 2021 •

edited

Loading

farshidtz commented May 25, 2021 •

edited

Loading

benfrancis commented May 31, 2021 •

edited

Loading

farshidtz commented May 31, 2021 •

edited

Loading