Things API with optional CRUD #273
Conversation
| }, | ||
| "base": "{{DIRECTORY_BASE_URL}}", | ||
| "tm:required": [ | ||
| "#/properties/things" | ||
| ], |
There was a problem hiding this comment.
The things property has two forms. One is for array payload which is mandatory, another is for the collection payload which is optional. To define this required form, the TM should support setting of a form to required using a nested required: true field. It currently doesn't.
I am setting the whole property to required, assuming that it means at least one form should offer this interaction. I can also set to #/properties/things/forms/0 but that means we are forcing the order of items in the forms array.
This was discussed in #253 (comment)
There was a problem hiding this comment.
TBH, I think that for now having only things property required is fine. I know that it is clearly depicting the whole picture but we have normative statements that cover that part. I would wait for the TD spec to catch up and deal with this kind of issue (I would say that the issue here is much more complex than just requiring that a derived TD must have a specific form).
There was a problem hiding this comment.
OK for now with this resolution, but I'm wondering now if we should not have two different interactions here. The expectation in TDs are that different forms are somehow equivalent, not for different purposes. The scripting API, for example, also assumes that any form in an interaction will do the same thing as any other.
There was a problem hiding this comment.
They are equivalent ways of getting the TDs. The only difference is payload and header formats.
There was a problem hiding this comment.
Discussion: Farshids thinks this is broken (maybe messed up as part of an earlier PR) and needs to be fixed. McCool thinks at the very least that putting forms distinguished by different contentTypes in a single interaction is inconvenient for Consumers. Farshid will create an issue and will investigate.
|
@RoboPhred, This PR closes your issue. I could not add you as the reviewer, but please consider doing so. |
index.html
Outdated
| If the missing feature is the whole functionality at an API endpoint, | ||
| the server may respond with 404 (Not Found). | ||
| Alternatively, a server may respond with 501 (Not Implemented) | ||
| to signify that it has placeholders in place for the given | ||
| endpoint and would provide this functionality in future. |
There was a problem hiding this comment.
I wasn't sure if this should be an assertion. It won't help the clients anyway because it is optional for servers. Turning it into an assertion also means that we need to find two implementations who don't implement the feature and use those status codes!
There was a problem hiding this comment.
I am not completely convinced about returning 501 either. Programmatically speaking having 404 or 501 result in the same output -> I cannot use this endpoint. About user, perspective knowing that the feature Might be implemented in the future feels like a temporary workaround. I would prefer chooseing just one of the two error codes.
There was a problem hiding this comment.
Removed 501 and converted 404 to an assertion, but recommended instead of optional.
There was a problem hiding this comment.
OK. I agree with only having one error code. I am personally unsure why we don't make this mandatory but having it as a recommended is ok (probably best to not force an implementation to do anything at all if there is an error).
There was a problem hiding this comment.
The "may" should probably be MAY though and marked as an assertion with a span and id, however. Are all errors written like this? Do we have a general statement/assertion somewhere about errors that makes individual assertions unnecessary?
There was a problem hiding this comment.
@mmccool I think you are commenting on the outdated diff.
Regarding mandatory or recommended; as you said, forcing this means server have to implement it specifically that way. For example, a server may need to implement all those HTTP handlers and return 404.
farshidtz
force-pushed
the
things-api
branch
from
f8456d6 to
479a87d
Compare
| <ul> | ||
| <li> | ||
| If the missing feature is to customize existing functionality of an | ||
| API, the server will respond with 501 (Not Implemented) |
There was a problem hiding this comment.
Assertions are in the respective sections as explained below:
The normative behavior is prescribed in the respective sections.
/thingsendpoint and justify optional "registration" interfaces; closes Read-Only Directories #208/eventsendpoint for consistencythingsproperty (listing interface) to required in TM and bump version; related to "Thing description" for Directory should be a "Thing Model" #86idcomes from; closes Small fixes on Creation #256After review in the discovery call on Feb 14th, 2022:
Another PR: Update section IDs - doing so will make section IDs and assertions IDs inconsistent. Updating assertion IDs will cause issues with existing testing tools.
Preview | Diff