Update Openapi Definition for tenant and device #1418
Conversation
Signed-off-by: wistefan <[email protected]>
| @@ -440,7 +440,12 @@ components: | |||
| Tenant: | |||
| type: object | |||
| additionalProperties: false | |||
| required: | |||
| - tenant-id | |||
There was a problem hiding this comment.
Please note that this is the OpenAPI spec for the HTTP based Device Registry Management API which is per se independent from the other (AMQP based) APIs that the adapters use, e..g Tenant, Credentials API. The information that adapters expect in responses to their requests needs to get into the registry somehow, so the payloads used with the Management API need to be semantically compatible with the payloads of the other APIs. However, they do not necessarily need to be syntactically identical.
That said, this property has been deliberately left out of the payload.
For a GET request, the client will already know the id because it has specified it in the URI path.
For a POST request it either knows the ID (using /tenants/${tenant-id} URI) or it doesn't and wants the registry to create one (using just /tenants URI). In the latter case, the generated ID is being returned as part of the location response header.
For a PUT request, the client has to specify the id in the URI path already.
There was a problem hiding this comment.
Hi, thanks for the explanation.
Maybe some background: we currently work on a frontend for the administration api and an integration between hono and hawkbit, based on the api. To do so, we would like to extend the api by 2 methods - getAllTenants and getAllDevicesForTenant. Since the current objects do not include the id, they cannot be used in this cases. We thought to include id would keep the object-model consistent. An alternative would be to introduce something like an IdentifiableDevice/IdentifiableTenant and use it in those methods.
Is this something you can imagine for the management api at all? And what would be the preferred way? I will file a PR with the complete changes, if that helps. We thought about doing the changes in parts, to get first experience with the API, before contributing it.
There was a problem hiding this comment.
I see your point. We already anticipated extensions of the existing API, in particular regarding additional query operations etc. However, we didn't think about the impact this would have on the payload. FMPOV it would make sense to add an (optional) tenant/device ID property to the respective schema objects for this purpose.
@ctron @jbtrystram @dejanb WDYT?
There was a problem hiding this comment.
I suppose a getAll method for tenants or devices for tenants would return an array of the already defined payloads, that would be reasonable.
In that case I agree with @sophokles73 , a tenant/device ID property in the payload would make sense. But it should not be mandatory, only optional.
To be more generic, it could be named "id" (as it is now done when using the auto-generated id feature)
There was a problem hiding this comment.
I don't think adding an "optional" ID field is a good idea. Because for some operation it just is not optional, but required.
We could however add a derived, extended FooWithId object, which simply adds the ID field, to the existing other fields.
There was a problem hiding this comment.
As far as I can see there's no consensus generally on how to approach this. Although, having an id optionally in the body is seen as good practice as it give the most flexibility to the API. Users can choose whether they are gonna put it in the body while creating a resource and the implementation should just make sure that it doesn't conflicts url provided ID if any. For returning values although it makes sense in most cases to provide an id in the results, this option doesn't enforce this on the implementation, which again provides more flexibility for demo implementations and such.
So +1 from me.
There was a problem hiding this comment.
However, this distinction will make it quite hard to implement the API because you will always need to filter out the objects' identifier property when returning a single instance
You are assuming that you are storing the ID alongside the payload. Which might not be the case.
Having the ID as part of the payload would mean that for store operations you would need to ensure that the correct ID is set. e.g. the user tries to update "objectA" and puts in "id=objectB".
Retrieving the object would mean that you either sore the ID alongside with the payload, or every fetch operation would need to parse the payload, add the field, and re-encode it afterwards.
It may be tempting to have this, but there are downsides to adding the ID as well.
There was a problem hiding this comment.
Having the ID as part of the payload would mean that for store operations you would need to ensure that the correct ID is set. e.g. the user tries to update "objectA" and puts in "id=objectB".
IMO we should avoid this situation. If the ID is added in the payload, then it should become the only parameter for the resource, thus we remove it from the URI path. (i.e. PUT on /tenants will use the id in the payload if it's provided, generate a value if not).
There was a problem hiding this comment.
So in some cases you provide the ID using the URI (delete, get) and in some others you provide it via the payload (create, update)? That sounds a bit odd to me.
And as we are trying to get 1.0 out the door this week, I am not sure there any benefit in doing this now.
There was a problem hiding this comment.
In our implementation we now went the way with using additional identifiable-objects, that include the ID. It would leave the current api untouched, but still gives all information required. Since its non-breaking, I do not think that it needs to be included in 1.0. I would propose to reject this PR and I file a new one with the full api after 1.0 is out.
sophokles73
added
Management API
|
Trying to sum up the contents of the discussion: |
I updated the tenant and device objects in the openapi definition, to include their respective id's. I also added "required" according to https://www.eclipse.org/hono/docs/api/tenant-api/.
Signed-off-by: wistefan [email protected]