Skip to content
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

Rename actions to match properties and events #160

Closed
wants to merge 3 commits into from

Conversation

benfrancis
Copy link
Member

@benfrancis benfrancis commented Apr 22, 2021

This PR proposes renaming and combining actions and their API endpoints in the Directory Service API to match other proposed changes to the example Thing Directory Description (in #158 and #159):

  • createTD -> createThing
  • updateTD & updatePartialTD -> updateThing
  • updateTD -> updateThing
  • updatePartialTD -> partialUpdateThing
  • deleteTD -> deleteThing

See discussion in #133


Preview | Diff

directory.td.json Show resolved Hide resolved
directory.td.json Show resolved Hide resolved
directory.td.json Outdated Show resolved Hide resolved
@benfrancis benfrancis changed the base branch from main to refactor April 30, 2021 16:25
@mmccool
Copy link
Contributor

mmccool commented May 3, 2021

Not merging for now, some concern about merging PATCH with PUT. It may be more RESTful, but is also more inconvenient since we don't have a good/convenient way to select forms in the scripting API. Two paths forward:

  1. Use actions (e.g. separate interactions) for updates as in the existing API
  2. Fix the scripting API to allow easy selection of forms by methods (but... protocol specific). Using different interactions (even if they map to the same URL in a given protocol) we can decouple the script from the protocol.

In general... making things more "RESTful" also seem to be making them more protocol specific and may be a step backwards. Just having one name (interaction) per distinct API operations seems... simpler.

@mmccool
Copy link
Contributor

mmccool commented May 3, 2021

Also, to be clear: I think the TD for the directory should be a TM and normative...
Right now it is an "example" for reasons but it is meant to be a normative definition of the API.
In particular, names of interactions should all be normative. All directories should "implement" the TM, including the names of the interactions as given in the Directory TM.

@relu91
Copy link
Member

relu91 commented May 4, 2021

some concern about merging PATCH with PUT

I would say that the problem relies on the fact that updating a TD using PATCH is not equal to update a TD using PUT. I think we should clarify in the TD spec (if not already) that the form listed for an affordance should be treated as equivalent to each other. Mining that they could be used interchangeably the perform the same logical operation.

This is not the case for substituting a TD (update using PUT) or modify part of a TD (update PATCH). The proof is that if a take a TD (which is also a valid PartialTD), the final output depends on operation chosen:

  • PUT the directory TD will be exactly equal to the TD provided as input
  • PATCH the directory TD will be equal to the TD provided as input plus previously defined fields that were not in the input

@benfrancis
Copy link
Member Author

I understand what you're getting at. But the PUT and PATCH in this API both correspond to a writeproperty operation. If they are considered different interactions, should there also be a separate operation type for partial writes (e.g. partialwriteproperty) in order to distinguish between those different interaction affordances?

@mmccool
Copy link
Contributor

mmccool commented May 10, 2021

We discussed in the Discovery TF meeting and generally agreed that the new names were a useful improvement, but disagreed on the merger of PUT/PATCH (especially if they are hard to distinguish in the scripting API, or make it harder to distinguish the functionality in other protocols/make the API more HTTP specific).

So... if we had a PR that just did the name changes, we could merge it. @benfrancis for now, would you mind updating this PR to take out the PUT/PATCH changes and leave the name changes? We can discuss PUT/PATCH separately.

@benfrancis
Copy link
Member Author

@mmccool I'm a bit confused about why it's OK for createThing to have both a PUT and a POST option, but updateThing can not have both a PUT and a PATCH option?

@farshidtz
Copy link
Member

@mmccool I'm a bit confused about why it's OK for createThing to have both a PUT and a POST option, but updateThing can not have both a PUT and a PATCH option?

As @relu91 explained above, PUT and PATCH accept different content types and do very different operations.

For createThing, both PUT and POST create a TD and the difference is whether or not the ID is user-defined. A mistake by the client will most probably result in an error. If necessary, we could move POST to a new interaction called createAnonymousTD.

@mmccool
Copy link
Contributor

mmccool commented May 17, 2021

  • PUT/POST: scripting API implementation (e.g. node-wot) needs to handle as part of registration; hidden from users.
  • PUT/PATCH: we don't have a partialwrite op, and so we can't distinguish these two at the scripting API level just using interaction/op combination. Technically you could use formIndex to pick one but it's ugly. Two possible solutions:
    1. Use different interaction name for PUT and PATCH
    2. Add an appropriate op type to the TD spec
    3. Add a hook to the scripting API to fix this

My opinion: 1 is the easy thing to do, 2 is the right thing to do, 3 is a hacky workaround. We could do both 1 and 2 (e.g. doing 1 does not preclude doing 2 later).

Actions:

  1. Create issue in TD spec about adding a partialwrite op to support PATCH (esp when in same interaction with a PUT). Will also want to raise issue in Scripting API.
  2. Split this PR into two: (1) does renamings we agree on; (2)(which we can defer until we decide we want it) that merges PUT and PATCH interactions. @farshidtz has agreed to do (1). If this is done carefully, then this PR will only have the changes for (2) left.
  3. @farshidtz will also do a PR to separate POST and PUT for registration, to have a "CreateAnonymousThing" action; then it would be an error to try to use "CreateThing" for a TD with no ID.

@benfrancis
Copy link
Member Author

benfrancis commented May 18, 2021

I've added a commit to re-separate update and partial update.

@benfrancis
Copy link
Member Author

Superseded by #184

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants