[BUG] Empty string id is not correctly validated by forceId

[simonbrunel]

Description/Steps to reproduce

• define a model with forceId: true
• associate this model to a mongodb data source
• call the API to create a new model with an explicit empty string id

curl -X POST --header "Content-Type: application/json" --header "Accept: application/json" -d "{
  \"id\": \"\"
}" "http://localhost:3000/api/foos"

• this call creates a db entry with empty id and returns: {"id": ""}

Seems related to #1453 (see my comment)

Link to reproduction sandbox

https://github.com/simonbrunel/loopback-sandbox/tree/bug/empty-id-mongodb

Expected result

{
  "error": {
    "name": "ValidationError",
    "status": 422,
    "message": "The `foo` instance is not valid. Details : `id` can't be set (value: \"\").",
    "statusCode": 422,
    "details": {
      "context": "foo",
      "codes": {
        "id": [
          "absence"
        ]
      },
      "messages": {
        "id": [
          "can't be set"
        ]
      }
    }
  }
}

Additional information

node v6.11.3, Windows 10 64bit

npm ls --prod --depth 0 | grep loopback
[email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected]

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[simonbrunel]

Sandbox sources: loopback-sandbox-bug-empty-id-mongodb.zip (repository deleted)

[bajtos]

@simonbrunel thank you for reporting this issue and sorry for responding so late!

Originally, I wanted to respond to your comment #1453 (comment) and say that I am not sure if your analysis of
our implementation is correct.

shouldn't we also reject any requests containing an empty string ID when forceId is true? validatesAbsenceOf considers an empty string value valid, which generates a record with no id (at least with MongoDB) when calling a POST endpoint with an explicit empty string ID even if forceId is true (I'm new to LoopBack, so I might have missed something).

IIUC, blank('') returns true, i.e. an empty string is considered as "blank". validateAbsence negates the result of blank, it triggers a validation error when blank returned true. Because empty string is considered blank, validateAbsence should trigger a validation error in such case.

I suspect the problem is caused by something else, either in LoopBack implementation or in the configuration of your application.

I am afraid I don't have enough time to investigate this myself :(

[simonbrunel]

@bajtos validateAbscence triggers an error if blank returns false (if (!blank(this[attr])) { err() }). IIRC, my issue was that if an empty string id was sent via POST (create), the record was created with an empty string as ObjectId (instead of a generated ObjectId) because of this line

if (forceId) {
  // if id is an empty string, validation passes because blank() returns true
  ModelClass.validatesAbsenceOf(idName, {if: 'isNewRecord'});
}

[bajtos]

@simonbrunel oh, you are right, I should not be analyzing code on Friday evening.

In this case, I have the following proposal:

1. Extend validatesAbsenceOf with configuration options like allowEmpty, see validatesNumericalityOf for an example: https://github.com/strongloop/loopback-datasource-juggler/blob/54143dfa37c1d32bc8c1d3fd3e79fd44def5bb48/lib/validations.js#L126-L127

2. Modify the implementation of forceId to leverage this new option and tell the validator to reject empty strings.

@rashmihunt @jannyHou @kjdelisle thoughts?

[Yaty]

Hi, any update on this issue ? I maybe will try to make a PR (if I have time).

[jannyHou]

@bajtos Your suggestion #1519 (comment) sounds good to me 👍

@Yaty Thank you! Feel free to open a PR for fix, I can review and help you land it!

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[bajtos]

Not stale, we have a pull request in progress: #1576