wvteijlingen / Spine

A Swift library for working with JSON:API APIs. It supports mapping to custom model classes, fetching, advanced querying, linking and persisting.
MIT License
266 stars 109 forks source link

Non-registered types in resource relationships break deserialization #88

Closed invliD closed 8 years ago

invliD commented 8 years ago

When trying to fetch resources that contain embedded relationships with a type that has not been registered with Spine (e.g. because it was unknown this type existed during development), Spine crashes the application with a failed assertion: ResourceFactory.swift:37

Resource example:

{
  "type": "content-items",
  "id": "1",
  "attributes": { ... },
  "relationships": {
    "content": {
      "data": {
        "type": "videos",
        "id": "1"
      }
    }
  }
}

In my use case, the type of the relationship can be various different objects (which fully works in Spine – as long as every single possible type is implemented and registered). Unfortunately, sometimes types are added and I need to prevent my app from crashing just because there's a new content type.

My proposal would be an additional boolean attribute on the Relationship fields (e.g. nilIfUnsupported) that would prevent the assertion and instead just use nil as the field's value.

I will submit a PR as soon as I have this implemented, please stop me if you think this is not a good idea (e.g. if your opinion is just always returning nil in this case, without the flag).

wvteijlingen commented 8 years ago

If I understand this right, content is a polymorphic relationship? I've never really tested polymorphic relationships, but afaik Spine does support them as long they all inherit from the same base resource.

How did you define the relationship field on your ContentItemsResource?

invliD commented 8 years ago

You're partially right, it does have different types, though they do not inherit from a common class right now (I could add that to my Spine models, but I don't think it should be necessary).

Right now I only have the following definition in the Spine model fields: "content": ToOneRelationship(Video), because I noticed the type argument will only ever be used if the type value is missing from the JSON-API endpoint (which should never happen, because it has to be there according to the specs). When the attribute is present (aka all the time) it actually uses that type to look up the model to instantiate: DeserializeOperation.swift:279. The attribute on the model is defined like so: var content: Resource?

wvteijlingen commented 8 years ago

Hmm yeah, I see the problem. I think that if we want to support this use case, best thing to do would be to throw an error in the ResourceFactory and catch that in the Deserialiser. Then when the specific type isn't registered, it could fall back on the type argument of the Relationship.

This requires using a common class, which I strongly encourage you to create anyway. The Router uses the type argument to build nested include paths (such as content.author.name), which will probably not work correctly without a common class.

invliD commented 8 years ago

Sounds good, I'll go ahead and implement that.