scoutforpets / jsonapi-mapper

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

Relation names are used for model types. #30

Closed ralfschimmel closed 8 years ago

ralfschimmel commented 8 years ago

First of all; great work 👍

However; I noticed that relation names are used as model types. For example; I have a model with two belongsTo's called goodGuy and badGuy of the type guy. The mapper returns JSON with related data as expected, though the type of the models is goodGuys and badGuys instead of guys for both.

jamesdixon commented 8 years ago

Hi, @ralfschimmel. Thanks 😄

I haven't run into this myself as all of my types map directly to the model. That said, it's a perfectly valid case. I believe that our code doesn't support this at the moment, but it certainly could be added to the roadmap. The potential issue, which we've faced previously, is that we're highly dependent on the information Bookshelf gives us in order to be able to map the model properly. That said, I'm not sure if information about a base type, such as guys is available in the model. Any chance you might be able to investigate to see if you can find this in the model before it's passed to the mapper? If so, then this would probably be a relatively trivial change to implement.

Cheers, James

chamini2 commented 8 years ago

I did the investigation and from my findings, it's not in the bookshelf model. I propose to check an attribute in the bookshelf model (maybe ._modelName) and if it's present use that name, if it's not present use the relation name.

ShadowManu commented 8 years ago

I highly discourage adding non-public (expected) properties to objects to work as a public API. Even less when the object is foreign (coming from the bookshelf package).We could instead play with the mapper instance options or the map method options.

jamesdixon commented 8 years ago

Agreed -- if this is going to be supported, it should probably be something that's passed to the mapper as an option.

@ralfschimmel did you end up finding a solution to this problem? Curious if it's something we could implement.

confuser commented 8 years ago

Just came across this issue myself. @jamesdixon What about using a virtual property to support this, such as type or _type?

ShadowManu commented 8 years ago

@confuser what do you think on extending the relations option to accept an object working as a map for name conversions? I think that is approachable and flexible.

confuser commented 8 years ago

Sounds like a viable solution. It does mean additional setup for each serialisation, rather than a behind the scene meta approach, but it would be the more flexible option.

ShadowManu commented 8 years ago

Since its a customization about the mapping/serialization process, I personally think its better to treat it in here than to delegate it to customizations on the bookshelf side.

ShadowManu commented 8 years ago

@confuser This functionality has been added as as new Mapper option, which you can see how it works here in the specs. I believe this covers the use case?

chamini2 commented 8 years ago

Fixed in d0c43e0fa0f7bcb7d3b546796e09d8057e9ea0cb , check the README and specs (and here) for the usage instructions.