search

scoutforpets / jsonapi-mapper

JSON API-Compliant Serialization for your Node ORM
The Unlicense
42 stars 24 forks source link

filters out camelCased foreign keys/allows for whitelisting attrs (fixes #56, #70) #71

Closed jamesdixon closed 8 years ago

chamini2 commented 8 years ago

I understand the idea of the regexes to remove attributes, but it never felt great to omit attributes that match certain patterns.

What do you think of removing this feature and instead offer an opposite option to omit attributes? So everyone has total control of what goes out and what doesn't. Right now the mapper makes a decision based on our opinion and the user would have to overwrite the functionality of the mapper through the attrsWhitelist.

I prefer the users to pass a list (or even maybe regexes to match like the restricted list!) of attributes to omit, that way making obvious the omission. Could save some time debugging.

chamini2 commented 8 years ago

Also it leaves questions like _"why are Id and _id attrs removed, but ID attrs are not?"_. And also, these changes could introduce unexpected behaviour to users that update the package.

jamesdixon commented 8 years ago

@chamini2 I think the id conventions are pretty typical, but I agree that it's opinionated. I'd be up for simply adding a restricted list that could be either a RegEx or a string.

jamesdixon commented 8 years ago

Also, it's strange: the test that fails on Travis runs just fine for me locally.

chamini2 commented 8 years ago

Also, it's strange: the test that fails on Travis runs just fine for me locally.

Apparently bookshelf 0.10.1 breaks this test. I don't know why tho.

chamini2 commented 8 years ago

@chamini2 I think the id conventions are pretty typical, but I agree that it's opinionated. I'd be up for simply adding a restricted list that could be either a RegEx or a string.

I'm not sure I understand what you meant, could you explain again please?

jamesdixon commented 8 years ago

@chamini2 I think the id conventions are pretty typical, but I agree that it's opinionated. I'd be up for simply adding a restricted list that could be either a RegEx or a string.

I'm not sure I understand what you meant, could you explain again please?

Sorry, I was agreeing with you. Essentially, I was saying that if you'd like to remove the default attr restrictions, that would be fine with me. We should then have some sort of "blacklist" that will omit specific fields. That list should allow for either regular expressions or strings.

chamini2 commented 8 years ago

Awesome! Yeah, we would need a omitAttrs or attrsBlacklist option. Could you implement this in another PR?

chamini2 commented 8 years ago

Let me know if you'll do it, if not, I can.

jamesdixon commented 8 years ago

Not sure when I'll get to it -- if you have time and want to implement, be my guest 👍

jamesdixon commented 8 years ago

@chamini2 if you do start working on this, just let me know so we don't overlap 👍 I'll do the same

chamini2 commented 8 years ago

I'll get it done. Closing this one.