mattpolzin / JSONAPI

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

chore: allow absent `data` in ToManyRelationship #110

Open nekrich opened 1 year ago

nekrich commented 1 year ago

Changes

Allow ToManyRelationship to have no data.

Make an early exit early from the initializer by setting an empty array to idsWithMeta if the container has no data key.

Reasoning

I've just started working with App Store Connect API. Their API conforms to JSON:API.

For starters, I want to list Bundle IDs. That API doesn't list data for relationships unless they are specified in the request include. But even specifying include is limited to one element only. Yes, it's an array, I didn't check it with Bundle ID yet, but I am sure about this behavior in Profiles API).

So, there is no data in relationships, but there are useful links to fetch relationships for each element.

Sample JSON:

{
  "data" : [ {
    "type" : "bundleIds",
    "id" : "bundleId_id",
    "attributes" : {
      "some" : "attributes"
    },
    "relationships" : {
      "bundleIdCapabilities" : {
        "meta" : {
          "meta": "data"
        },
        "links" : {
          "related" : "https://api.appstoreconnect.apple.com/v1/bundleIds/bundleId_id/bundleIdCapabilities"
        }
      },
      "profiles" : {
        "same": "as bundleIdCapabilities"
      }
    }
  } ]
}
mattpolzin commented 1 year ago

Would the MetaRelationship type described here work for your use-case?

nekrich commented 1 year ago

Would the MetaRelationship type described here work for your use-case?

Unfortunately, it will not work. When I specify include parameter for the request, I receive relationships with data. Using MetaRelationship and ToManyRelationship means I need to declare a second ResourceObjectDescription with relationships with data, but they are the same.

nekrich commented 1 year ago

The same list request but with additional parameters returns the same JSON as above + data 😥. Inventing Either or copying structs doesn't seem like a reasonable option for me.

"data" : [ {
    "type" : "bundleIds",
    "id" : "bundleId_id",
    "attributes" : {
      "some" : "attributes"
    },
    "relationships" : {
      "bundleIdCapabilities" : {
        "meta" : {
          "meta": "data"
        },
        "links" : {
          "related" : "https://api.appstoreconnect.apple.com/v1/bundleIds/bundleId_id/bundleIdCapabilities"
        }
        "data" : [ {
          "type" : "bundleIdCapabilities",
          "id" : "id2"
        }, {
          "type" : "bundleIdCapabilities",
          "id" : "id2"
        } ],
mattpolzin commented 1 year ago

I'm not totally ruling out the option you've coded up here, because it does feel like a very appropriate use-case, but I am hesitant to make this change because it means that in situations where I want to assert that I expect all relationships to be returned in the response data I would not have a way to tell the difference between (a) the response payload indicated 0 relations are present on the object and (b) the response payload omitted the data altogether (possibly because I made a mistake with my request or because the server code has an bug).

Maybe some way to make the handling you've introduced in this PR optional, so I can choose between turning on "handle omitted data as empty array" or not (the default being to error out)?

nekrich commented 1 year ago

I’ve got your point. I can think of two options:

mattpolzin commented 1 year ago

I can think of two other options:

I think I probably lean toward one of EmptiableToManyRelationship (we might be able to name it better, but I like the idea) or a static configuration type.

The nice thing about the protocol idea is that it targets individual relationships which could be useful compared to toggling the behavior for a whole project.

nekrich commented 1 year ago

I've got a mix 😅.

You can't access the previous coding key while decoding an exact relationship object, so there will be no access to a static on Relationships level.

Another impossible thing is generic type determination while decoding.

So, I've got a flag protocol with a static variable inside. Tesource description object must confirm this protocol. Made inherited conformance in the ResourceObject, and finally was able to determine the canHaveNoDataInRelationships flag while decoding. The default value for canHaveNoDataInRelationships is false.

The solution is dynamic, so any time you can turn on/off this behavior for the same relationship type 🎉.

And added default conformance of ResourceObjectProxyDescription to ResourceObjectWithOptionalDataInRelationships for seamless integration.

mattpolzin commented 7 months ago

Getting back to this because I'd like to get it merged. I realized my last comment may have been ambiguous (did I want you to make the change or would I be making it?) -- apologies if you were waiting on me!

I've addressed my feedback now. After giving this another full read-through, I am noticing that because ResourceObjectProxyDescription conforms to ResourceObjectWithOptionalDataInRelationships and ResourceObjectDescription conforms to ResourceObjectProxyDescription, ResourceObjectWithOptionalDataInRelationships is not really a flagging protocol: it is conformed to by 100% of all resource objects. That doesn't break existing functionality since the default for canHaveNoDataInRelationships is false, but it does seem at odds with the intended design. If it is always going to be there, it may as well become a new addition directly on ResourceObjectProxyDescription.

I may do some thinking on that here today. If you have thoughts on it, please let me know! I fully understand if you've moved on from this PR after so long, though.