search

vadimdemedes / mongorito

🍹 MongoDB ODM for Node.js apps based on Redux
1.38k stars 90 forks source link

Allow hiding attributes from toJSON() #114

Closed mrusme closed 8 years ago

mrusme commented 8 years ago

I was looking for a way to hide specific attributes inside the toJSON() transformation of a model and quickly implemented something here. Let me know what you think of it. If you're okay with the implementation, I can extend the PR with documentation + tests. Regards!

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.8%) to 93.213% when pulling bfb87dc31213205c9f3fa2f9110d078683d9d1f6 on twostairs:master into e6f05e7768bf6510da35ed635fd4a7c1b4b45602 on vdemedes:master.

vadimdemedes commented 8 years ago

Thank you for your efforts, but I don't think it should be inside Mongorito, because toJSON() is kind of a standard way to provide serialized data. Check out toJSON() in Backbone for a reference.

mrusme commented 8 years ago

@vdemedes thank you for your reply! Can you think of any better place to integrate such a feature? The use case I'm trying to solve is: My Model contains an attribute named password, which (of course) doesn't need to be exported to JSON (-> returned via API). Therefor I tried to build a generic way to allow the exclusion of any attribute from being exported to JSON. The idea for this was basically taken from Laravel's Eloquent.

vadimdemedes commented 8 years ago

I'd suggest adding a new method specifically for this use case, like toFilteredJSON or something.

ulpian commented 8 years ago

@mrusme Why not just leave it as an option in queries or perhaps in the collection class; attributes: false?

mrusme commented 8 years ago

@vdemedes what would be the actual benefit? I only see the downside of being unable to work with models as before, requiring to fulfill(model.toFilteredJSON()) instead of simply fulfill(model). Besides, I'm not sure whether a separation would actually make sense here, as toJSON would only filter, if hidden-attributes were defined.

@ulpian it's not query related, that's why. Maybe I didn't understand your suggestion correctly, but we're talking about the model toJSON conversion here. It wouldn't make sense to set this inside the actual query, as the model internal data wouldn't be affected of this. This means: One definitely wants to have the password hash inside the model, so one can work with it (e.g. hash it using the entered password to see, whether it's correct) - one might just not want to have to return the data like this:

let data = clone(model.get());

delete data.password;

fulfill(data);

As stated before, I think the more elegant solution here would be having it handled automatically be the generic toJSON method for the model, if hidden-attributes where configured inside the model's configure() method.

mrusme commented 8 years ago

I've created a new PR in order to be able to continue the work with my actual project: https://github.com/vdemedes/mongorito/pull/120 - sorry for inconveniences!