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

Safe cloning user object for further serialization #11

Conversation

chrisbujok
Copy link
Contributor

@chrisbujok chrisbujok commented Dec 21, 2016

Hello,

I tried to use this module with my feathers app and feathers-sequelize Users service, but functions sanitizeUserForClient and sanitizeUserForNotifier cause error:

TypeError: Converting circular structure to JSON
    at Object.stringify (native)
    at stringify (/***/node_modules/express/lib/response.js:1064:12)
    at ServerResponse.json (/***/node_modules/express/lib/response.js:243:14)
    at Object.applicationJson [as application/json] (/***/node_modules/feathers-rest/lib/index.js:27:11)
    at ServerResponse.res.format (/***/node_modules/express/lib/response.js:628:13)
    at formatter (/***/node_modules/feathers-rest/lib/index.js:25:7)
    at Layer.handle [as handle_request] (/***/node_modules/express/lib/router/layer.js:95:5)
    at next (/***/node_modules/express/lib/router/route.js:131:13)
    at Object.callback (/***/node_modules/feathers-rest/lib/wrappers.js:87:14)
    at callbackWrapper (/***/node_modules/rubberduck/lib/rubberduck.js:50:20)
    at /***/node_modules/feathers/lib/mixins/promise.js:35:14

I think this is because Object.assign() does not copy values like custom toJSON method, which is used e.g. in Sequelize model objects and is necessary for proper object serialization.

I came up with idea of checking for existence of custom toJSON method in user object and then using it's result as a source for new user object. It happens only when JSON.stringify() throws error, so in my opinion - only when necessary.

The caveat is that after object sanitization, sequelize methods inside model object will not be accessible, but this is the same situation as current.

Please let me know what do you think.

@eddyystop
Copy link
Collaborator

Thanks for the catch.
I somehow missed this PR for 3 days. Sorry.

MongoDB has a similar issue. It uses a .toObject function instead of .toJSON.
Perhaps cloneObject is more descriptive name that cloneUserObject.

Would you mind making these 2 changes to the PR?

@chrisbujok chrisbujok force-pushed the safely-sanitize-circural-objects branch from bb08bef to 9e538ac Compare December 30, 2016 09:42
@chrisbujok
Copy link
Contributor Author

Sorry, I just found some time to fix this.
I've added support for toObject but toJSON has precedence over it.

@eddyystop
Copy link
Collaborator

+1 Thanks

@eddyystop eddyystop merged commit d51c8b3 into feathersjs-ecosystem:master Dec 30, 2016
@eddyystop
Copy link
Collaborator

Pushed with 0.1.6

@chrisbujok chrisbujok deleted the safely-sanitize-circural-objects branch January 2, 2017 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants