Incorrect swagger definition for models with forceId:true

[bajtos]

Description of feature (or steps to reproduce if bug)

The swagger specification generated for LoopBack models with forceId set to true (the default in LoopBack 3.0) is incorrect. The model definition marks the id property as required, but the create operation is rejecting requests with id property set.

This causes problems in API Connect, where the sample request data generated by Editor contains id now.

Expected result

The sample request generated by APIC Editor for create operation does not contain id property.

Actual result (if bug)

The sample request generated by APIC Editor for create operation contains id property.

Additional information (Node.js version, LoopBack version, etc)

• loopback@3
• loopback-swagger@latest

The default value of forceId was changed to true in LoopBack 3.0 for security reasons, see loopbackio/loopback-datasource-juggler#982 and loopbackio/loopback-datasource-juggler#849

[bajtos]

The petstore example shows the approach we can use too.

https://github.com/OAI/OpenAPI-Specification/blob/master/examples/v2.0/yaml/petstore-expanded.yaml

Check out their POST /addPet - it takes a body of type NewPet. GET /pet returns a type Pet, which is allOf NewPet, plus id.

[bajtos]

Instead of hard-coding detection of forceId, we should come up with a property flag indicating whether a property may or may not be in create data. This flag then can be used e.g. for Cloudant's _rev field, see loopbackio/loopback-connector-cloudant#55

[ritch]

This is not a blocker for 3.x because we can fix this in the swagger generator.

[bajtos]

Instead of hard-coding detection of forceId, we should come up with a property flag indicating whether a property may or may not be in create data. This flag then can be used e.g. for Cloudant's _rev field, see loopbackio/loopback-connector-cloudant#55

Similarly, we need to specify that User.create body must contain a required password property, that's otherwise hidden from all other payloads.

[bschrammIBM]

This issue is producing a 422 error in the Testing a LoopBack project tutorial. It is causing confusion for the customers. Any chance it could get fixed?

[bajtos]

@bschrammIBM Thank you for bringing this issue into our attention. Could you please give us the link to "Testing a LoopBack project tutorial" you have mentioned?

@raymondfeng @ritch @kjdelisle ping

[bschrammIBM]

Here is the tutorial mentioned: https://www.ibm.com/support/knowledgecenter/SSMNED_5.0.0/com.ibm.apic.toolkit.doc/tutorial_cli_project_test.html   I have added the following note which will be in a future release of the Knowledge Center (unless this issue gets fixed!):   The generated data sample requires editing in order to produce a sample response. Remove the line for "id" and the comma at the end of the previous line. The sample code will look similar to the following: { "type":"ATM", "phone":"(854)509-3813"}   Yours, Barbara ----- Original message -----From: Miroslav Bajtoš <[email protected]>To: strongloop/loopback <[email protected]>Cc: bschrammIBM <[email protected]>, Mention <[email protected]>Subject: Re: [strongloop/loopback] Incorrect swagger definition for models with forceId:true (#2924)Date: Thu, May 25, 2017 8:22 AM  @bschrammIBM Thank you for bringing this issue into our attention. Could you please give us the link to "Testing a LoopBack project tutorial" you have mentioned? @raymondfeng @ritch @kjdelisle ping —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.  

[jannyHou]

Instead of hard-coding detection of forceId, we should come up with a property flag indicating whether a property may or may not be in create data. This flag then can be used e.g. for Cloudant's _rev field

@bajtos I agree with this solution 👍 , how about name the flag as readOnly? since those properties would be generated by database, user could only read it but not write.

[jannyHou]

@bschrammIBM Thanks! I will work on the fix asap

[jannyHou]

Task:

• Add a property flag, when turning on, swagger doesn't mark it as required.
• Make sure apic designer and loopback explorer don't show those fields with flag: true in create, update, replace methods.
• Test cases
• Add the flag in our doc

[bajtos]

how about name the flag as readOnly? since those properties would be generated by database, user could only read it but not write.
make sure apic designer and loopback explorer don't show those fields with flag: true in create, update, replace methods.

I am afraid this issue is more complex.

• The fields id and _rev must not be present in the payload of create operation, but they must be present in the payload of update and replace methods. I feel readOnly is not capturing this intent very well. Perhaps generated: true may be better?

• The field password must be present in create, but not in update and replace. I am not sure what flag name to use, maybe createOnly?

[dhmlau]

Sorry that I might be new to this topic...

From the documentation, https://loopback.io/doc/en/lb3/Embedded-models-and-relations.html, I thought forceId is to force the generation of id, so theoretically, it shouldn't be required in the create operation.
It seems like that's not the case. but wouldn't the solution be making sure the forceId option does what it's supposed to do?

Unless we want to specify this kind of behavior beyond id, then I agree with @bajtos ' comment above.

[crandmck]

@dhmlau I believe this is a different forceId property that is part of the relation definition.

It's unfortunate that it has the same name as the top-level forceId property being discussed here, which is documented in https://loopback.io/doc/en/lb3/Model-definition-JSON-file.html#top-level-properties.

[dhmlau]

From @bajtos

I think the tutorial is referring to the following page: https://www.ibm.com/support/knowledgecenter/SSFS6T/com.ibm.apic.toolkit.doc/tutorial_cli_project_test.html

[rashmihunt]

• Separated out the work done in Sprint 41 into https://github.com/strongloop-internal/scrum-asteroid/issues/275
• keeping this item for next sprint sprint and re-estimated for the remaining work - mainly, adding few more tests, fixing test case regressions, PR review and npm publish all 3 repos.

[rashmihunt]

Making it to 8 points based on the issue found during the review and left over work

[rashmihunt]

Will be closing this issue since all the PRs for this feature are merged and published to npm

• Flag id as updateOnly when forceId is in effect loopbackio/loopback-datasource-juggler#1453
• Support createOnlyInstance in model #3548
• Support updateOnly feature loopback-swagger#92
• Test case change for updateOnly property loopback-sdk-angular#276

@bschrammIBM please open a doc issue to remove the workaround added in the documentation which talks about removing the 'id" field explicitly from the JSON input to avoid validation error. @bajtos wondering if API designer picks up latest version of this from npm or it needs to change anything from the designer side (loopback-workspace version?) to pick up latest version from npm?

[bajtos]

wondering if API designer picks up latest version of this from npm or it needs to change anything from the designer side (loopback-workspace version?) to pick up latest version from npm?

IIRC, API Connect locks down the dependencies to the latest version at the time a new APIC version is published. In which case a new release of apiconnect is need to ship this fix to our customers. Perhaps @kraman can help us here?

[crandmck]

Since @bschrammIBM is on vacation atm, I created the doc issue https://github.ibm.com/apimesh/apiconnect-docs/issues/395.
I assume she'll address it when she returns (tomorrow).

[dhmlau]

Per @crandmck 's comment, assign this to @bschrammIBM as well.

[bschrammIBM]

@rashmihunt
Can this be closed?