search

eclipse-hono / hono

Eclipse Hono™ Project
https://eclipse.dev/hono
Eclipse Public License 2.0
451 stars 137 forks source link

Inconsistent naming in device registration API JSON (case style) #1910

Open calohmn opened 4 years ago

calohmn commented 4 years ago

In the Device Registry Management API, the fields in the Device JSON structure related to the gateway groups feature are defined in camel-case notation:

  "viaGroups": [
    "string"
  ],
  "memberOf": [
    "string"
  ],

However, in the other parts of the API, the kebab-case notation is used (e.g. auth-id, minimum-message-size, resource-limits, etc.).

sophokles73 commented 4 years ago

Not sure what the raised thumbs are supposed to be indicating. In any case I would be in favor to consistently use kebab-case, i.e. change viaGroups to via-groups and memberOf to member-of. We need to be careful, though, to not introduce a breaking API change since this affects public API ...

ctron commented 4 years ago

So my :+1: was for fixing the inconsistency by using "kebab-case".

mhemmeter commented 4 years ago

Some additional thoughts from my side: We should also look at the other Eclipse IoT projects and their API definition. Among others Eclipse Ditto is of interest because of our Eclipse IoT Packages and they are following camel-case notation (see for example https://www.eclipse.org/ditto/http-api-doc.html#/Things/get_things__thingId_).

From my perspective we should coordinate and go for a consistent approach. WDYT? @thjaeckle: Any thoughts from your side?

thjaeckle commented 4 years ago

@mhemmeter yes, Ditto uses camel case in its JSON formats. And having that as public released API, we can't change this. So from my point of view when we want to have consistent apis in scope of Eclipse IoT Packages, newly added apis should also be on camel case 😉

mhemmeter commented 4 years ago

From my perspective it is important that we are consistent across the projects to improve UX and bring Eclipse IoT forward as a whole. So the community should at least decide on the preferred way for the future. It was important for me to bring this up here as the discussion was going in the direction of kebab-case.

When we will be consistent is another question but indeed we probably end up with a new major version of the APIs - may it be in Eclipse Hono or Eclipse Ditto.

thjaeckle commented 4 years ago

From my perspective it is important that we are consistent across the projects to improve UX and bring Eclipse IoT forward as a whole.

I disagree in the way that this isn't important enough to break already existing APIs. Built applications on top of that API would have to change "just because" we now like kebap better than camel case (for example, or the other way around). Newly added APIs should be IMO

sophokles73 commented 2 years ago

I agree with @thjaeckle wrt to the most important thing being to use consistent case across a project's APIs. Whether or not the usage of camel vs. kebab case should be harmonized across the Eclipse IoT working group projects is another discussion.

@thjaeckle The terms tenantId and deviceId are being used as URI parameter names but not in the JSON schemas. We might want to change these names to use kebab case as well, but this change would have no impact on any client using the API.

So for Hono, this boils down to change the (only) two occurrences of camel case being used in the Device schema to camel case as proposed above.

That said, even this very small change will already break the existing API. We are currently preparing Hono 2.0.0 so this might be a chance to actually do the change. On the other hand, I wonder if this (cosmetic) change really justifies the effort that we would need to spend in order to provide for

Maybe it would be better to stick with this minor annoyance for some time longer until we decide to make some substantial breaking changes to the management API and then fix this as well.

WDYT? @calohmn @kaniyan @ctron @dejanb

calohmn commented 2 years ago

@sophokles73 FMPOV, introducing a new major API version just for these 2 property changes doesn't seem worth the effort. So I would also say to rather postpone this change to be done later along with other changes.

I guess the commit https://github.com/eclipse/hono/commit/3e7b3768a09e9e57cfe43af5b9a5432a8849af96 should be reverted then, removing the device-registry-v2.yaml file. Otherwise we would need to add /v2/ endpoints now.

sophokles73 commented 2 years ago

If we all agree that we should stick to v1 for the time being, then yes, we should revert the commit. @kaniyan @dejanb @ctron any other opinion?

kaniyan commented 2 years ago

@sophokles73 FMPOV it is not worth the effort considering the change and what value it brings. I would also postpone it to another time.

sophokles73 commented 2 years ago

Ok, then I will unschedule this issue from 2.0.0