Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for JSON arrays #3228

Merged
merged 3 commits into from
Dec 23, 2015
Merged

Added support for JSON arrays #3228

merged 3 commits into from
Dec 23, 2015

Conversation

Hanifb
Copy link

@Hanifb Hanifb commented Sep 14, 2015

Fixes the issue #2977, which sails would convert an
array to an object and subsequently fail validation.

Fixes the issue #2977, which sails would convert an
array to an object and subsequently fail validation.
@tjwebb
Copy link
Contributor

tjwebb commented Sep 15, 2015

nice work @Hanifb

@sgress454
Copy link
Member

@Hanifb awesome--a test for this in /test/integration/router.APIScaffold.test.js would allow us to merge...

Added testing for JSON arrays
@Hanifb
Copy link
Author

Hanifb commented Oct 3, 2015

@sgress454 Done!

dominicrico added a commit to dominicrico/sails-hook-sockets that referenced this pull request Oct 12, 2015
Pass array in POST to body instead of building object to handle it right in the sails main app.
@tjwebb
Copy link
Contributor

tjwebb commented Oct 13, 2015

@dominicrico @Hanifb can you guys come up with a test that ensures this scenario passes?

@dominicrico
Copy link

@Hanifb are you doing this?

@Hanifb
Copy link
Author

Hanifb commented Oct 13, 2015

Already did, se my PL on sails-hook-sockets: balderdashy/sails-hook-sockets#10

@dominicrico
Copy link

👍

vazarely added a commit to vazarely/sails that referenced this pull request Dec 11, 2015
@sgress454 sgress454 merged commit eb62c9b into balderdashy:master Dec 23, 2015
@@ -40,7 +40,7 @@ module.exports = function createRecord (req, res) {
Model.subscribe(req, newInstance);
Model.introduce(newInstance);
}
Model.publishCreate(newInstance.toJSON(), !req.options.mirror && req);
Model.publishCreate(newInstance, !req.options.mirror && req);
}
Copy link
Member

Choose a reason for hiding this comment

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

@Hanifb What was the thinking behind this? Is it somehow necessary to get arrays to work? By pushing this logic into publishCreate, we force users of that method to pass in model instances instead of being able to send in plain objects. Sorry I didn't catch this earlier.

Copy link
Author

Choose a reason for hiding this comment

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

@sgress454 Wow, i dont even remember. Actually it is kinda unneccessary, because the publishCreateSingle//former publishCreate already does that.

Copy link
Author

Choose a reason for hiding this comment

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

@sgress454 Well now i remember, the reason is because how pluralize method works, it needs an instance of a model. Do you think we should remove the call to pluralize and instead use a simple if(!_.isArray(instance)){ [instance] }?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants