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

Update operations do not update the td id correctly #18

Merged
merged 8 commits into from
Sep 6, 2022

Conversation

ivanzy
Copy link
Contributor

@ivanzy ivanzy commented Aug 25, 2022

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"
}

@relu91 relu91 changed the title 14 update operations do not update the td id correctly Update operations do not update the td id correctly Aug 26, 2022
Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I just have a little comment. I don't like the name of the newly introduced method checkDuplicateThingDescriptionId, it is verbose. What about assertUniqueID ?

@ivanzy
Copy link
Contributor Author

ivanzy commented Aug 26, 2022

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
Copy link
Member

relu91 commented Aug 26, 2022

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)

Comment on lines 134 to 138
private async checkDuplicateThingDescriptionId(urn: string, id: string | undefined): Promise<void> {
if (!id || id === urn) return;
const idExist = await this.thingDescriptionRepository.exist({ where: { urn: id } });
if (idExist) throw new DuplicateIdException(id as string);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function signature could become clearer by renaming the parameters urn and id to something like currentId and newId.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Member

@hyperloris hyperloris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ivanzy
Copy link
Contributor Author

ivanzy commented Sep 6, 2022

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 .

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thing, update the contributing.md with this new guideline. Then we can close also #19

@ivanzy
Copy link
Contributor Author

ivanzy commented Sep 6, 2022

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
Copy link
Member

relu91 commented Sep 6, 2022

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

@relu91 relu91 merged commit 4513dd9 into main Sep 6, 2022
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.

3 participants