Skip to content

Fix remoting metadata for "data" arguments [2.x] - #2702

Merged
bajtos merged 1 commit into
2.xfrom
fix/data-object-arguments
Sep 7, 2016
Merged

Fix remoting metadata for "data" arguments [2.x]#2702
bajtos merged 1 commit into
2.xfrom
fix/data-object-arguments

Conversation

@bajtos

@bajtos bajtos commented Sep 2, 2016

Copy link
Copy Markdown
Member

Fix the definition of "data" argument to

{ type: 'object', model: modelName, ... }

That way strong-remoting passed the request body directly to the model
method (does not create a new model instance), but the swagger will
still provide correct schema for these arguments.

This fixes a bug where upsert in relation methods was adding default
property values to request payload.

Connect to #1806
Requires also strongloop/loopback-swagger#55 to be released around the same time

@0candy please review

This patch clashes with #2539 which is in review too. We need to rebase the second patch to be landed after the first one is merged.

@bajtos bajtos added this to the #Epic: Coercion Cleanup milestone Sep 2, 2016
@bajtos bajtos added the #review label Sep 2, 2016
@bajtos bajtos changed the title Fix remoting metadata for "data" arguments Fix remoting metadata for "data" arguments [2.x] Sep 2, 2016
@0candy

0candy commented Sep 2, 2016

Copy link
Copy Markdown
Contributor

@bajtos The code change LGTM. But does this code need to be in 3.x?

@bajtos

bajtos commented Sep 5, 2016

Copy link
Copy Markdown
Member Author

But does this code need to be in 3.x?

I'll forward-port to 3.x later. I started with a patch sent against 2.x to see how downstream dependencies are handling the change. There are very few dependents using LooBack 3.x. It has happened to us very recently that we landed a patch to 3.x because all looked well and then there was a bunch failures after back-porting to 2.x

@0candy 0candy assigned bajtos and unassigned 0candy Sep 6, 2016
Fix the definition of "data" argument to

    { type: 'object', model: modelName, ... }

That way strong-remoting passed the request body directly to the model
method (does not create a new model instance), but the swagger will
still provide correct schema for these arguments.

This fixes a bug where upsert in relation methods was adding default
property values to request payload.
@bajtos
bajtos force-pushed the fix/data-object-arguments branch from 70f50af to fcfdb73 Compare September 7, 2016 11:00
@bajtos

bajtos commented Sep 7, 2016

Copy link
Copy Markdown
Member Author

I compared swagger documents produced before this patch and after this patch. The only difference is that with this patch in place, the responses for relation methods include the description from remoting metadata, which is a good thing IMO.

(...)
@@ -267,6 +270,7 @@
           description: Model instance data
           required: false
           schema:
+            description: Model instance data
             $ref: '#/definitions/Category'
       responses:
         '200':
@@ -373,6 +377,7 @@
           description: An object of model property name/value pairs
           required: false
           schema:
+            description: An object of model property name/value pairs
             $ref: '#/definitions/Category'
         - name: id
           in: path
@@ -399,6 +404,7 @@
(...)

@bajtos
bajtos merged commit 69103d5 into 2.x Sep 7, 2016
@bajtos
bajtos deleted the fix/data-object-arguments branch September 7, 2016 12:09
@bajtos bajtos removed the #review label Sep 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants