feat(repository): rework *ById methods to throw if id not found #1723
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bajtos
force-pushed
the
fix/update-delete-not-found
branch
from
9bffc20 to
ef8b9f1
Compare
bajtos
force-pushed
the
fix/update-delete-not-found
branch
from
ef8b9f1 to
08a8553
Compare
Rework Repository interface and implementations to throw EntityNotFound error from `updateById`, `replaceById` and `deleteById` when there is no entity with the provided id. Update related controller methods to return 204 No Content on success, because there is no boolean flag to indicate success anymore. Clients should check the response status code instead (204 vs 404). This also reverts commit f68a45c.
bajtos
force-pushed
the
fix/update-delete-not-found
branch
from
08a8553 to
c5888ee
Compare
jannyHou
reviewed
Sep 18, 2018
| const idProp = this.modelClass.definition.idName(); | ||
| const where = {} as Where; | ||
| where[idProp] = id; | ||
| return this.updateAll(data, where, options).then(count => count > 0); | ||
| const count = await this.updateAll(data, where, options); |
There was a problem hiding this comment.
we don't return Oops never mind. I just realize we returned a boolean instead of the count anymore?count itself.
jannyHou
reviewed
Sep 18, 2018
| @@ -216,11 +221,11 @@ export class DefaultCrudRepository<T extends Entity, ID> | |||
| return this.toEntity(model); | |||
| } | |||
|
|
|||
| update(entity: T, options?: Options): Promise<boolean> { | |||
| update(entity: T, options?: Options): Promise<void> { | |||
There was a problem hiding this comment.
I think it should call
return await this.updateById(entity.getId(), entity, options)same for the next function delete.
There was a problem hiding this comment.
return await is actually slightly less performant than return because of promise re-wrapping.
See http://exploringjs.com/es2016-es2017/ch_async-functions.html#_returned-promises-are-not-wrapped
jannyHou
reviewed
Sep 18, 2018
| return this.updateById(entity.getId(), entity, options); | ||
| } | ||
|
|
||
| delete(entity: T, options?: Options): Promise<boolean> { | ||
| delete(entity: T, options?: Options): Promise<void> { |
There was a problem hiding this comment.
return await this.deleteById(entity.getId(), options);
hacksparrow
approved these changes
Sep 18, 2018
raymondfeng
approved these changes
Sep 18, 2018
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up for #1658 related to #1469.
Rework Repository interface and implementations to throw EntityNotFound error from
updateById,replaceByIdanddeleteByIdwhen there is no entity with the provided id.Update related controller methods to return 204 No Content on success, because there is no boolean flag to indicate success anymore. Clients should check the response status code instead (204 vs 404).
Improve our REST layer to set the response status code to 204 when body is undefined.
This patch is reverting the implementation details of #1621 while keeping the observed REST API behavior.
See the discussion in #1658 and #1658 (comment) in particular to better understand the reasoning for this change. Also note that soon we will add a new Repository implementation that will provide
*ByIdvariant using a a special return value to indicate "not found" case - see #1735.Checklist
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated