scoutforpets / jsonapi-mapper

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

pivot (join) table attributes not included in response #17

Open jamesdixon opened 8 years ago

jamesdixon commented 8 years ago

When utilizing a belongsToMany relationship in Bookshelf, the _pivot fields are not being included in the response.

chamini2 commented 8 years ago

Should they be added like meta attributes or as part of the normal attributes?

jamesdixon commented 8 years ago

I'd imagine they'd be rendered as a standard relationship. As an example, if you had an appointment, service and a join table appointment_service, the pivot would be appointment_service, which is really just another relationship.

chamini2 commented 8 years ago

I was imagining it like if you're returning as the main the appointment, then in the relationships there would be services, and add in the services meta some more attributes.

jamesdixon commented 8 years ago

I think someone did mention about using meta to do this. Honestly, it's probably not worth implementing at the moment until we get some additional feedback.

Given it's not explicitly defined in the spec, I believe we need to see how front-ends are consuming it before we implement. Are you running into this issue specifically?

chamini2 commented 8 years ago

Are you running into this issue specifically?

No, I was just curious. You're right, not worth implementing yet.

jamesdixon commented 8 years ago

@chamini2 FYI I came across a case for this in my application.

Essentially, I need to utilize some of the additional data in a join/pivot table in my application. It appears the suggested way to do this is through the use of a meta attribute.

For example, in my application, I have an appointment model that contains a hasMany relation to service. For each service being performed, they may apply to all pets or just a specific set of pets. The table that stores which services are associated with an appointment is the appointment_service table, which contains appointment_id and service_id. However, I need a way to say that those services may apply to a specific subset of pets. The ideal solution is to have another field in the join table; let's call it specificPets.

When requesting an appointment with the service relation, I'd place the specificPets attr as part of the relations's meta data. Example:

{
    "data": {
        "type": "appointment",
        "attributes": {
            "startsAt": "2016-03-09T12:00",
            "endsAt": "2016-03-09T12:30",
            "businessNotes": "Be careful -- Critter likes to try and get out the door when it's opened.",
            "customerNotes": "Critter has been having the shits. Keep an eye on him."
        },
        "relationships": {
            "pets": {
                "data": [{
                    "type": "pet",
                    "id": 1
                }]
            },
            "services": {
                "data": [{
                    "type": "service",
                    "id": 1
                }],
                "meta": {
                    "specificPets": [1, 2]
                }
            }
        }
    }
}

Ember Data (allegedly one of the first implementations of consuming JSON API) supports this out of the box. For reference, here's their documentation: https://guides.emberjs.com/v2.8.0/models/handling-metadata/

My proposal is that we serialize _pivot attributes into the "meta" attribute. More details TBD.

chamini2 commented 8 years ago

We have the same case (obviously not for pets) and handled it with a meta too. The thing is that we didn't include it in the GET it was just for a POST, so no need to customize the serializer from us.


A question, shouldn't it be:

{
    "data": {
        "type": "appointment",
        "attributes": {
            "startsAt": "2016-03-09T12:00",
            "endsAt": "2016-03-09T12:30",
            "businessNotes": "Be careful -- Critter likes to try and get out the door when it's opened.",
            "customerNotes": "Critter has been having the shits. Keep an eye on him."
        },
        "relationships": {
            "pets": {
                "data": [{
                    "type": "pet",
                    "id": 1
                }]
            },
            "services": {
                "data": [{
                    "type": "service",
                    "id": 1,
                    "meta": {       // <-------- inside the element
                        "specificPets": [1, 2]
                    }
                }]
            }
        }
    }
}
jamesdixon commented 8 years ago

I thought that too at first, but it actually sits one level up: http://jsonapi.org/format/#document-resource-object-relationships

chamini2 commented 8 years ago

And what if you had a service to be done to pets 1,2 and another service for pets 2,3. Which I understand your current DB schema permits.

jamesdixon commented 8 years ago

It would just be another service object under the relationship data.

chamini2 commented 8 years ago

But what would the meta be to represent it?

jamesdixon commented 8 years ago

Ah, sorry. I see what you're saying. Good question!

chamini2 commented 8 years ago

Maybe the meta goes in the included instead of the relationships part for this?

jamesdixon commented 8 years ago

Related: https://github.com/SeyZ/jsonapi-serializer/issues/78

jamesdixon commented 8 years ago

As that thread mentioned, it could be done by adding everything needed to the meta object. Although, it's definitely cleaner being able to embed it within the relation item.

jamesdixon commented 8 years ago

Maybe the meta goes in the included instead of the relationships part for this?

Not sure that it would be compliant w/ the spec.

chamini2 commented 8 years ago

They say it's ok to use the meta inside the data part

jamesdixon commented 8 years ago

Yeah, I did see that. The only reason I wouldn't go that route is because Ember Data follows the spec's recommendations. That said, in order to use it the other way, I'd have to submit a PR to Ember Data and not sure it's worth the effort :)

chamini2 commented 8 years ago

The cleaner way I see is adding it in the included resource object, but you would have to include the relationships always.

jamesdixon commented 8 years ago

The cleaner way I see is adding it in the included resource object

Can you give an example?

but you would have to include the relationships always.

This isn't ideal for my case. I selectively include when necessary, but leave it up to Ember Data to use the relationships information to lazy load when possible.

chamini2 commented 8 years ago
{
    "data": {
        "type": "appointment",
        "attributes": {
            // info
        },
        "relationships": {
            "services": {
                "data": [{ "type": "service", "id": 1}, { "type": "service", "id": 2}]
            }
        }
    },
    "included" : [
      {
        "type": "service",
        "id": 1,
        "meta": {
          "specificPets": [1,2]
        },
        "attributes": {
          ...
        }
      },
      {
        "type": "service",
        "id": 2,
        "meta": {
          "specificPets": [2,3]
        },
        "attributes": {
          ...
        }
      }
    ]
}
jamesdixon commented 8 years ago

Certainly possible, but I don't like that we'd be forcing the inclusion of relationships in order to make this work.

Are you opposed to following the spec as-is for the time being with the option to add more meta functionality down the road?

chamini2 commented 8 years ago

Are you opposed to following the spec as-is for the time being with the option to add more meta functionality down the road?

You propose to leave this as is for now and consider it later or to implement it somehow?

jamesdixon commented 8 years ago

No, I'm proposing that we add the meta object outside the data element as is currently specified in the spec. It would at least provide some level of metadata for relations. Note that with regard to pivot data specifically, it would only apply to belongsToMany relations.

chamini2 commented 8 years ago

Then sure!, if it's compliant I'm in.