search

vaimee / zion

A scalable Thing Description Directory
Apache License 2.0
17 stars 4 forks source link

Update operations do not update the td id correctly #18

Closed ivanzy closed 2 years ago

ivanzy commented 2 years ago

When a TD is updated (PUT or PATCH), the internalThingDescription urn is updated as well. Additionally, there is an additional check to ensure that the new id is not a duplicated one. The new exception is: 

{
  "type": "/errors/types/duplicate-id",
  "title": "Duplicate Id",
  "status": 409,
  "detail": "The id {id} is already in use by another Thing Description"
}
ivanzy commented 2 years ago

I would say it is better to be verbose than vague. assertUniqueID does not specify which id it verifies the uniqueness, so I would name it assertThingDescriptionUniqueId, which is as verbose as the current name.

Don't be afraid to make a name long. A long descriptive name is better than a short enigmatic name. A long descriptive name is better than a long descriptive comment. Robert C. Martin

relu91 commented 2 years ago

Ok then I think we have a consensus here: assertUniqueThingDescriptionID (I think putting Unique first sounds better in English but I am not sure how to apply for the English advective force order here: The rule is that multiple adjectives are always ranked accordingly: opinion, size, age, shape, colour, origin, material, purpose ) .

I've also noticed that we have requireValidThingDescription method in the same class. We should decide whether to use assert or require in such cases and write the final decision here (I'm opening an issue to tracking this down)

ivanzy commented 2 years ago

The code is great. Thanks for the work! If possible, it would be important to add a couple of failing tests (for both the PUT and PATCH) when the client tries to change the TD id to an existing one. Basically check the correct behavior of the checkDuplicateThingDescriptionId in the respective two cases.

Done in commit #2dc14bb .

ivanzy commented 2 years ago

To maintain consistency, I will open another branch to do it. That way, we have a 1-to-1 relationship between issues, branches, and PRs.

relu91 commented 2 years ago

Ok, I thought that in 55f7688 you updated also the other methods. Then it's ok as it is!