feat: default 404 for delete request to non-existent resource #1621

hacksparrow · 2018-08-20T15:08:24Z

Send 404 for delete requests to non-existent resource by default. Addresses issues like #1472

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

raymondfeng · 2018-08-20T15:11:52Z

packages/repository/src/repositories/legacy-juggler-bridge.ts

@@ -128,7 +128,7 @@ export class DefaultCrudRepository<T extends Entity, ID>
    this.modelClass = dataSource.createModel<juggler.PersistedModelClass>(
      definition.name,
      properties,
-      Object.assign({strict: true}, definition.settings),
+      Object.assign({strict: true}, {strictDelete: true}, definition.settings),


It's simpler to have Object.assign({strict: true, strictDelete: true}, definition.settings),.

Ah, indeed.

hacksparrow · 2018-08-20T15:15:50Z

In LB3, users can do <Model>.settings.strictDelete = false to override the default. Not sure how we would do this in LB4. Besides, we don't have the equivalent/corresponding page for Model definition JSON file, yet.

virkt25 · 2018-08-21T00:00:41Z

packages/repository/src/repositories/legacy-juggler-bridge.ts

@@ -128,7 +128,7 @@ export class DefaultCrudRepository<T extends Entity, ID>
    this.modelClass = dataSource.createModel<juggler.PersistedModelClass>(
      definition.name,
      properties,
-      Object.assign({strict: true}, definition.settings),
+      Object.assign({strict: true, strictDelete: true}, definition.settings),


Hmm ... Based on the discussion we had in the issue I'm thinking it's fine to leave the code change out and just document how to set the strictDelete property on a model and what it means. A quick and simple change to the docs in this section (similar to the strict property): https://loopback.io/doc/en/lb4/Model.html#using-legacy-juggler should be enough imo. Thoughts?

Yep, I am fine with that too. How do you do <Model>.settings.strictDelete = false in LB4, though?

The same way you do it for strict: false in LoopBack 4.

Here's an example:

// Use strictDelete to return 404 if a delete fails. @model({settings: {strictDelete: true}}) class Todo extends Entity { ... }

And now we can leave strictDelete as default instead of making it true.

hacksparrow · 2018-08-22T15:06:09Z

@virkt25 @raymondfeng good to land?

hacksparrow · 2018-08-23T12:56:57Z

Any idea why coverage/coveralls would be affected by this?

bajtos

LGTM. As I wrote in #1472 (comment), I think it may be a good idea to change the default of strictDelete to true now that we have a chance to make breaking changes.

virkt25 · 2018-08-24T02:55:32Z

Any idea why coverage/coveralls would be affected by this?

I'm not sure but been wondering the same thing since I've noticed the coverage fluctuating on just .md file changes ... even though coveralls is showing no decrease in a file when I look at the report.

virkt25 · 2018-08-24T02:56:45Z

@hacksparrow Can you update this PR to make strictDelete the default and them the documentation should be updated to show users how to turn it off. Thanks. And just ping everyone for another review after.

hacksparrow · 2018-08-24T13:28:01Z

docs/site/Model.md

-  cart: ShoppingCart;
+  @property() email: string;
+  @property() isMember: boolean;
+  @property() cart: ShoppingCart;


VSC has applied these code formatting automatically. Is it OK, or should I exclude these changes?

Ideally, we should preserve the current style and revert these changes.

Could you please check that you have the latest version of the Prettier plugin for VS Code and that the plugin is configured to use the prettier version from the project? You may want to run npm run lerna:clean && npm install to ensure you have the latest version of prettier installed.

hacksparrow · 2018-08-24T13:46:10Z

Time for another round of review.

bajtos · 2018-08-24T13:53:09Z

packages/repository/test/acceptance/repository.acceptance.ts

+  it('enables strict delete by default', async () => {
+    const p = await repo.create({slug: 'pencil'});
+    p.name = 'Red Pencil';
+    await repo.save(p);


repo.create has already created a persisted instance. No need to call repo.save. Please remove L76 and L77.

bajtos · 2018-08-24T13:54:14Z

packages/repository/test/acceptance/repository.acceptance.ts

+
+    await pencilRepo.create({
+      name: 'Green Pencil',
+    } as AnyObject);


Why is it necessary to explicitly cast the value to AnyObject? In the test above, you called repo.create({slug: 'pencil'}) with no type casts.

hacksparrow · 2018-08-24T15:29:06Z

Made the previous change suggested by @bajtos. Anything else, anyone?

virkt25 · 2018-08-24T15:29:17Z

packages/repository/test/acceptance/repository.acceptance.ts

+      name: 'Green Pencil',
+    });
+
+    await expect(pencilRepo.deleteById(10000)).to.be.fulfilledWith(false);


Why is it fulfilledWith(false) ... shouldn't it be true?

It is in strictDelete: false mode, and it cannot find id 10000; instead of rejecting it resolves with false.

That's why default strictDelete: false is confusing.

It is in strictDelete: false mode, and it cannot find id 10000; instead of rejecting it resolves with false.

Could you please capture this information in a code comment?

I am expecting more readers to be confused about .fulfilledWith(false).

hacksparrow · 2018-08-27T09:21:46Z

@bajtos good to land?

bajtos

LGTM 👍

bajtos · 2018-08-27T09:59:41Z

packages/repository/test/acceptance/repository.acceptance.ts

+      name: 'Green Pencil',
+    });
+
+    await expect(pencilRepo.deleteById(10000)).to.be.fulfilledWith(false);


It is in strictDelete: false mode, and it cannot find id 10000; instead of rejecting it resolves with false.

Could you please capture this information in a code comment?

I am expecting more readers to be confused about .fulfilledWith(false).

send 404 for requests to non-existent resoure by default.

bajtos

Thanks 👍

hacksparrow self-assigned this Aug 20, 2018

hacksparrow requested review from bajtos and raymondfeng as code owners August 20, 2018 15:08

hacksparrow changed the title ~~fix: fix DELETE with invalid ID is successful #1472~~ [WIP] fix: fix DELETE with invalid ID is successful #1472 Aug 20, 2018

raymondfeng reviewed Aug 20, 2018

View reviewed changes

hacksparrow force-pushed the fix/#1472 branch from b567ff8 to f037504 Compare August 20, 2018 15:18

virkt25 reviewed Aug 21, 2018

View reviewed changes

bajtos mentioned this pull request Aug 21, 2018

Monthly Milestone - August 2018 🍉⛱ #1581

Closed

31 tasks

hacksparrow force-pushed the fix/#1472 branch 3 times, most recently from bed8a75 to edd77d7 Compare August 22, 2018 15:04

hacksparrow changed the title ~~[WIP] fix: fix DELETE with invalid ID is successful #1472~~ docs: document the behavior reported in #1472 Aug 22, 2018

virkt25 approved these changes Aug 22, 2018

View reviewed changes

shimks approved these changes Aug 22, 2018

View reviewed changes

raymondfeng approved these changes Aug 22, 2018

View reviewed changes

hacksparrow force-pushed the fix/#1472 branch from edd77d7 to 2bf0861 Compare August 22, 2018 16:02

bajtos approved these changes Aug 23, 2018

View reviewed changes

bajtos changed the title ~~docs: document the behavior reported in #1472~~ docs: document the behavior of delete operations on non-existent resources Aug 24, 2018

hacksparrow force-pushed the fix/#1472 branch from 2bf0861 to 3b12108 Compare August 24, 2018 11:52

hacksparrow changed the title ~~docs: document the behavior of delete operations on non-existent resources~~ feat: default 404 for request to non-existent resource Aug 24, 2018

hacksparrow force-pushed the fix/#1472 branch 2 times, most recently from 9129ed0 to 0c7bfaa Compare August 24, 2018 13:25

hacksparrow commented Aug 24, 2018

View reviewed changes

bajtos reviewed Aug 24, 2018

View reviewed changes

hacksparrow force-pushed the fix/#1472 branch from 0c7bfaa to 025331c Compare August 24, 2018 15:22

virkt25 reviewed Aug 24, 2018

View reviewed changes

hacksparrow force-pushed the fix/#1472 branch from 025331c to 0726f13 Compare August 27, 2018 04:53

hacksparrow changed the title ~~feat: default 404 for request to non-existent resource~~ feat: default 404 for delete request to non-existent resource Aug 27, 2018

hacksparrow force-pushed the fix/#1472 branch from 0726f13 to 2308e8a Compare August 27, 2018 08:46

bajtos approved these changes Aug 27, 2018

View reviewed changes

feat: default 404 for request to non-existent resource

ddb8882

send 404 for requests to non-existent resoure by default.

hacksparrow force-pushed the fix/#1472 branch from 2308e8a to ddb8882 Compare August 27, 2018 10:12

bajtos approved these changes Aug 27, 2018

View reviewed changes

hacksparrow merged commit f68a45c into master Aug 27, 2018

hacksparrow mentioned this pull request Aug 27, 2018

[REST] DELETE with invalid ID is successful #1472

Closed

bajtos mentioned this pull request Aug 31, 2018

fix: return 404 when a model was not found #1658

Merged

7 tasks

bajtos mentioned this pull request Sep 18, 2018

feat(repository): rework *ById methods to throw if id not found #1723

Merged

7 tasks

bajtos deleted the fix/#1472 branch October 4, 2018 06:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: default 404 for delete request to non-existent resource #1621

feat: default 404 for delete request to non-existent resource #1621

hacksparrow commented Aug 20, 2018 •

edited

Loading

raymondfeng Aug 20, 2018

hacksparrow Aug 20, 2018

hacksparrow commented Aug 20, 2018 •

edited

Loading

virkt25 Aug 21, 2018

hacksparrow Aug 21, 2018

virkt25 Aug 21, 2018

hacksparrow commented Aug 22, 2018

hacksparrow commented Aug 23, 2018

bajtos left a comment

virkt25 commented Aug 24, 2018

virkt25 commented Aug 24, 2018

hacksparrow Aug 24, 2018

bajtos Aug 24, 2018

hacksparrow commented Aug 24, 2018

bajtos Aug 24, 2018

bajtos Aug 24, 2018

hacksparrow commented Aug 24, 2018

virkt25 Aug 24, 2018

hacksparrow Aug 24, 2018

hacksparrow Aug 24, 2018 •

edited

Loading

bajtos Aug 27, 2018

hacksparrow commented Aug 27, 2018

bajtos left a comment

bajtos Aug 27, 2018

bajtos left a comment

feat: default 404 for delete request to non-existent resource #1621

feat: default 404 for delete request to non-existent resource #1621

Conversation

hacksparrow commented Aug 20, 2018 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacksparrow commented Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacksparrow commented Aug 22, 2018

hacksparrow commented Aug 23, 2018

bajtos left a comment

Choose a reason for hiding this comment

virkt25 commented Aug 24, 2018

virkt25 commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacksparrow commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacksparrow commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacksparrow Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacksparrow commented Aug 27, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

hacksparrow commented Aug 20, 2018 •

edited

Loading

hacksparrow commented Aug 20, 2018 •

edited

Loading

hacksparrow Aug 24, 2018 •

edited

Loading