mattpolzin / JSONAPI

Swift Codable JSON:API framework
MIT License
75 stars 19 forks source link

Add metadata for Resource Object Identifier in Relationships #82

Closed mlomeli closed 4 years ago

mlomeli commented 4 years ago

I have a BatchDocument that read Posts, which reference MediaObjects, which references Files.

My Post, includes the MediaObject which has a Relationship to a file called field_image that has the meta information inside the data.

public struct MediaImageDescription: ResourceObjectDescription  {
.
.
.
    public struct Relationships: JSONAPI.Relationships {
         var fieldImage: ToOneRelationship<DrupalFileMetadataSource, FileMetadata, NoLinks>
     }
                "field_image": {
                    "data": {
                        "type": "file--file",
                        "id": "redacted",
                        "meta": {
                            "alt": "Krillin sentado.",
                            "title": "Krillin",
                            "width": 1163,
                            "height": 2000
                        }
                    },

It didn't work until I finally could make it read it by modyfing the ToOneRelationship to this. Currently this works because this is my only use of metadata.

          if let noMeta = NoMetadata() as? MetaType {
                 meta = noMeta
             } else {
                 let nestedContainer = try container.nestedContainer(keyedBy: ResourceLinkageCodingKeys.self, forKey: .data)
                 meta = try nestedContainer.decode(MetaType.self, forKey: .meta)
             }

I'm new to the JSONApi spec, my question is, my API is out of spec. Or is this a current bug on the Library? Anyways I'll try to look it up or submit a patch when I'm finished with this project

Anyways thanks for your library and your help!

mattpolzin commented 4 years ago

Hi @mlomeli. I think I see what is going on here, but it would help a lot to see a more full example of the JSON payload you are parsing. Can you share the rest of the JSON payload where you got the field_image from above?

mlomeli commented 4 years ago
{
  "data": {
    "type": "people",
    "id": "88223",
    "attributes": {
      "first_name": "Lisa",
      "last_name": "Offenbrook",
      "age": null
    },
    "relationships": {
      "pets": {
        "data":
          {
            "type": "dogs",
            "id": "123",
            "meta": {
                "nickname": "Sparks."
            }
          }
      }
    }
  },
  "included": [
    {
      "type": "dogs",
      "id": "123",
      "attributes": {
        "name": "Sparky"
      }
    }
  ]
}

Here is an example. I ran it through an online validator and says it's valid. But couldn't get it to compile unless I make the change I pointed out, which looks for the metadata inside the data in the relationship.

Now that I look at it, this is probably a feature request.

Here it is the code I used to test this.

TestModel.txt

mattpolzin commented 4 years ago

OK, that's really helpful. Thanks. It took me a while looking at it to even spot what was going on, but this is indeed a part of the JSON:API specification that this JSONAPI library does not yet allow for. The reason it took me a while to spot is because this library does support metadata at the Relationship Object level but I had not even noticed that metadata is allowed inside the Resource Identifier Object as well.

In other words, the library already supports this:

{
  "data": {
    "type": "people",
    "id": "88223",
    "attributes": {
      ...
    },
    "relationships": {
      "pets": {
        "data":
          {
            "type": "dogs",
            "id": "123"
          },
          "meta": {
              "nickname": "Sparks."
          }
      }
    }
  },
  ...
}

But it does not support the equally valid meta location from your example.

If you would like, I will change this ticket over to a feature request and one of us can tweak the title to be a request for Resource Identifier Object metadata support.

I'm not sure when I will get around to adding this support because I am quite busy lately and this may or may not be easy to represent -- it is almost certainly a breaking change, at least. To do this properly it should be incorporated into the Id type.

mlomeli commented 4 years ago

Thanks, yeah doesn't seem like an easy fix. I can't change the tag for the feature request. I'll see if I can tackle it, once I finish my current project.

mattpolzin commented 4 years ago

@mlomeli are you planning on taking this task on? If not, I may start working on it myself pretty soon here. No pressure, I just don't want to duplicate effort.

mlomeli commented 4 years ago

I currently can't. If I do, it will be after October 15. But I'll reach out to contribute since this library is Godsend with to me, since I use Contenta CMS (Drupal 8) which offers JSONAPIs out of the box,

mattpolzin commented 4 years ago

No problem. I've got a draft implementation going over here but it will be a breaking change and major version bump.

This does not really belong in the JSONAPI library's Id type, it turns out, because it only applies to Resource Identifier Objects which are only found in Relationship Objects whereas the JSONAPI library uses its Id type in both relationship and resource object contexts. Just a little aside because I originally speculated this change would end up inside the Id type.

mattpolzin commented 4 years ago

Available in https://github.com/mattpolzin/JSONAPI/releases/tag/5.0.0-rc.1