Skip to content

WIP:Expose replace methods 2.x - #2330

Closed
Amir-61 wants to merge 5 commits into
2.xfrom
expose_replace_methods_2.X
Closed

WIP:Expose replace methods 2.x#2330
Amir-61 wants to merge 5 commits into
2.xfrom
expose_replace_methods_2.X

Conversation

@Amir-61

@Amir-61 Amir-61 commented May 11, 2016

Copy link
Copy Markdown
Member

Updated:

Description:
  • updateAttributes and updateOrCreate endpoints use PATCH (forward compatibility) and PUT(backward compatibility) verb by default .
  • Exposed replaceById and replaceORCreate via POST verb by default.
  • Add a flag model.settings.options.replaceOnPUT for 2.X to get 3.X behaviour

Note: This patch is for 2.X to be backported.

TODO:
  • Add a test which confirms the functionality of flag replaceOnPUT for 2.x

Related to #2316 (Implementation for 3.x)

Connect to #1979

@bajtos

bajtos commented May 11, 2016

Copy link
Copy Markdown
Member

Hey @Amir-61, before I review the code changes, I'd like to discuss the description:

updateAttributes and updateOrCreate endpoints use PATCH verb by default .

I think they should be mapped on both PUT and PATCH. PUT is needed for backwards compatibility, PATCH is for forward compatibility with LB 3.0

Thoughts?

Exposed replaceById and replaceORCreate via PATCH verb by default.

This does not make much sense to me. I would expect these two methods to be exposed as POST /api/mymodel/:id/replace and POST /api/mymodel/replaceOrCreate.

What do you think?

Add a flag model.settings.options.newMapping for 2.X to get 3.X behaviour

In my opinion, newMapping does not describe the intent/effect very well. How about replaceOnPUT?

Also I think the flag should be set directly on model.settings, I don't think there is an options property.

Comment thread lib/persisted-model.js Outdated
* @param {Error} err Error object; see [Error object](http://docs.strongloop.com/display/LB/Error+object).
* @param {Object} instance Repalced instance.
*/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this methods has a different signature:

function replaceById(id, data, cb);

Doesn't it? Please update jsdoc comment too.

@bajtos

bajtos commented May 11, 2016

Copy link
Copy Markdown
Member

Please add some unit-tests calling the new REST API endpoints, and also tests to verify the new config options.

@Amir-61 Amir-61 assigned Amir-61 and unassigned bajtos May 11, 2016
@superkhau

superkhau commented May 11, 2016

Copy link
Copy Markdown
Contributor

Add a flag model.settings.options.newMapping for 2.X to get 3.X behaviour

@bajtos Do you think its worth maybe doing all 2.x settings in one setting for futureproofing? I'm proposing something like:

if (model.settings.options.putMapping === '2.x')

//and in the configs
...
"options": {
  "putMapping": "2.x"
},
...

Then we can list the options by default when scaffolding models, etc and have it self documenting. Users can simply change the config and get the behaviour without having to dig through too much documentation (only for what is "putMapping v2"). Thoughts?

@Amir-61

Amir-61 commented May 12, 2016

Copy link
Copy Markdown
Member Author

Closing this PR in favour of #2316

Lets discuss the options for backward compatibility and forward compatibility in the other PR

@Amir-61 Amir-61 closed this May 12, 2016
@Amir-61 Amir-61 removed the #review label May 12, 2016
@Amir-61
Amir-61 deleted the expose_replace_methods_2.X branch May 12, 2016 01:05
@Amir-61
Amir-61 restored the expose_replace_methods_2.X branch May 13, 2016 01:33
@Amir-61 Amir-61 reopened this May 13, 2016
@Amir-61

Amir-61 commented May 13, 2016

Copy link
Copy Markdown
Member Author

I think they should be mapped on both PUT and PATCH. PUT is needed for backwards compatibility, PATCH is for forward compatibility with LB 3.0

Thoughts?

@bajtos
ummmm on the second thought, it seems this task requires very different implementation between 2.x and 3.x, so I re-open this PR to work on the implementation for 2.x. You may review the other patch for 3.x PR: #2316

@Amir-61 Amir-61 mentioned this pull request May 13, 2016
@Amir-61 Amir-61 changed the title Expose replace methods 2.x WIP:Expose replace methods 2.x May 13, 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.

3 participants