chore: allow absent data in ToManyRelationship

[nekrich]

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]

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

[nekrich]

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]

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]

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]

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

• Add a specific key to JSONEncoder and use context. Looks a little bit obscure.
• Add a protocol EmptiableToManyRelationship and check if confirms - ignore data. Looks more viable.

[mattpolzin]

I can think of two other options:

• Add a singleton static configuration type a la https://github.com/mattpolzin/OpenAPIKit/blob/release/3_0/Sources/OpenAPIKit/CodableVendorExtendable.swift#L24
• Support a new static function on the Relationships protocol that returns a list of relationships for which data is omittable.

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]

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.

[nekrich]

This is for test_ToManyRelationshipWithMetaNoDataNotOmittable to check the exact thrown error.

[mattpolzin]

This looks like a good way to get what you need. I'd like to make the requested change to the test, but other than that I approve.

I may want to tweak the name of the protocol or method when I go to write documentation for this before cutting the next release of the library, but that can wait until I am actually sitting there writing the docs because that's when I'll probably decide what sounds most intuitive. The bones of this implementation make sense.

[mattpolzin]

I would prefer to keep the coding keys private, not do an @testable import, and test the description of the error instead of the exact coding key it outputs. This is just a preference of mine, I don't think there's a right or wrong way to go about testing this.

The test could use the XCTAssertThrowsError to clean up a bit of the XCTFail work done above and it would look something like:

    XCTAssertThrowsError(
      try decodedThrows(type: ToManyWithMeta.self,
                        data: to_many_relationship_with_meta_no_data)
    ) { error in 
      XCTAssertEqual(error.localizedDescription, "The data couldn’t be read because it is missing.")
    }

[mattpolzin]

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.