CrudRestComponent returns 404 when trying to create a new resource via PUT

[baarney]

If I create a minimal app using the REST CRUD example, which uses the CrudRestComponent, then PUTting a new Todo returns a 404, when I believe it should create the resource:

https://tools.ietf.org/html/rfc7231#section-4.3.4

The PUT method requests that the state of the target resource be
created or replaced with the state defined by the representation
enclosed in the request message payload.

(emphasis mine)

Steps to reproduce

1. Create an example rest-crud app:

$ npm install -g @loopback/cli
$ lb4 example rest-crud
$ cd loopback4-example-rest-crud
$ npm start

2. Send the following HTTP request:

PUT http://localhost:3000/todos/101
{
    "id": 101,
    "title": "Todo 101",
    "desc": "Test CRUD PUT",
    "isComplete": false
}

Current Behavior

• Response is a 404:

{
    "error": {
        "statusCode": 404,
        "name": "Error",
        "message": "Entity not found: Todo with id 101",
        "code": "ENTITY_NOT_FOUND"
    }
}

• Todo is not created.

Expected Behavior

• Response is a 201.
• Todo is created.

Additional information

$ node -e 'console.log(process.platform, process.arch, process.versions.node)'
linux x64 14.0.0

$ npm ls --prod --depth 0 | grep loopback
@loopback/[email protected] /var/data/tmp/loopback4-example-rest-crud
├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]
├── [email protected]

Related Issues

Whilst this bug is specifically about the CrudRestComponent, the following all mention upsert or createOrReplace, which seems to be the core of the issue.

2550 - Add upsert functionality to DefaultCrudRepository
1920 - Feature parity with LoopBack 3.x
2273 - How to replaceById/replaceOrCreate with HasManyRepository

2550 in particular seems to have been accepted as a valid case, but then was eventually closed as nothing happened on it for 6 months.

[jannyHou]

looking...

[jannyHou]

@baarney

then PUTting a new Todo returns a 404,

PUT /todos/{id} aims to replace an existing record, please note the id is a required parameter. It calls replaceById underneath.

So I would say "PUTting a new Todo" is a wrong use case, and that's why it gets 404 returned.

And thank you for finding out #2550, yes that's the feature story we need: upsert. Do you mind if I reopen that one and close this as a duplicate? We try to gather discussion in one place.

[baarney]

Thanks for looking into this.

PUT /todos/{id} aims to replace an existing record

I think that's where we differ. The standard is well defined: PUT should create the resource if it does not exist, so I guess it comes down to whether or not conformance to standards is one of the goals? If it's not, then fine, but it would be good to document that somehwere.

Also, if the CrudRestComponent does not allow this, I can't see any way of creating a new resource with a client-supplied id. In the generated example, the Todo model id property is defined with generated: false, but there appears to be no way for client to create a new instance with a supplied id:

• PUTting a new resource with a supplied id fails as above
• POSTing a new resource with an id in the body fails with:

"code": "VALIDATION_FAILED",
...
"message": "should NOT have additional properties",
"info": {
  "additionalProperty": "id"
}

Do you mind if I reopen that one and close this as a duplicate? We try to gather discussion in one place.

Getting all the discussion in a single place is certainly good, and I think #2550 is probably a better place for that, so very happy to move the implementation disussion there.

But I don't think this is a duplicate, as there are two separate actions:

1. Adding a new method to the repository class.
2. Changing CrudRestComponent to use that new method.

and it is certainly possible to do (1) without doing (2).

If this is an invalid case and CrudRestComponent is working as designed, then I think it would be better to close it as won't fix or similar.

Cheers,

[baarney]

#788 is probably also relevant (I've updated the expected behaviour from 204 to 201).

If this is a valid case then the response should be 201 for a new resource and 204 for an updated resource.

[achrinza]

I think the main issue right now is that PUT is a "compound" action, meaning that it requires multiple database queries. This breaks ACID compliance. A workaround would be to utilize the Transactions API, but not all connectors (notably MongoDB) support this feature.

Hence the current setup is a trade-off of not supporting the "correct" operation in favour of giving the most diverse support to the different connectors.

We could technically introduce a flag to support "correct PUT", trading off support for connectors such as MongoDB. However, a consideration would be that the rest-crud package is meant to be a simple, lightweight package. If a user wants full compliance, they could consider using the full Model-Controller-Repository instead of rest-crud.

[baarney]

Can we re-open #2550 and move the technical discussion there, as I think there are two separate questions:

1. Whether the default repo should support upsert/createOrReplace.
2. Whether CrudRestComponent should use that for its PUT oprtation.

And discussing (2) almost certainly depends on the outcome of (1).

[achrinza]

For the first point, I believe the direction is to not support such "compound" repository functions and remove existing implementations in favour of explicit transactions. See #897.

[baarney]

Thanks for those references. After reading those, may as well close this as won't fix.

I'm disappointed that LB decided to drop support for updateOrCreate in #897 as it is a fundamental database operation. By making that choice you are just pushing the problem out to the client, which goes against the whole point of using a framework. But you've had that discussion and made the decision, so no problem.

What I do find extremely frustrating is auto-closing of requests through inactivity (#2550). If that had been resolved as wont-fix with a reference to #897, it would have saved us all a lot of time!

Appreciate all the work put in to LB4 though - still an excellent core framework.

[stale]

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

[stale]

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.