Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "UNIVERSAL" case to BundleIDPlatform enum #197

Closed
wants to merge 1 commit into from
Closed

Add "UNIVERSAL" case to BundleIDPlatform enum #197

wants to merge 1 commit into from

Conversation

marcprux
Copy link
Contributor

@marcprux marcprux commented Aug 9, 2022

In the ASC docs for BundleIDPlatform and in the code generated from app_store_connect_api_2.0_openapi.json spec, BundleIdPlatform is an enum that can be "IOS" or "MAC_OS". However, the ASC servers break this by sometimes returning "UNIVERSAL".

This was noticed and corrected in 2020 in #70, but it appears that the additional case that was manually added was then eventually lost in a refactoring.

The discrepancy between the spec and the data returned the servers is out of our control, and this highlights an underlying brittleness to using frozen enums to represent enumerations that may get additional cases added to them in the future. Any enum that starts returning an unhandled case will break the entire parse, making forward-compatible clients impossible.

I think the long-term solution to this issue would be to have an option of generating an enum-simulating RawRepresentable wrapper around the string with some convenience public static accessors initializer. For example, something like:

struct BundleIdPlatform : RawRepresentable, Coddle { 
    var rawValue: String
    static let ios = Self(rawValue: "IOS")
    static let macos = Self(rawValue: "MAC_OS")
}

extension BundleIdPlatform {
    // unhandled cases can be manually added with an extension
    static let universal = Self(rawValue: "UNIVERSAL")
}

Support for this would be need to be implemented in https://github.com/CreateAPI/CreateAPI, since their enum handling code seems hardwired (and unaffected by my setting generateEnums: false in .create-api.yml, as well as adding the "UNIVERSAL" case to the rename.enumCases property).

This short term patch simply manually adds in the undocumented "UNIVERSAL" case, but since this will need to be done every time the API is re-generated, it is not a great solution.

For the record, here is an example of the parse failure when the server returns the unhandled case:

<unknown>:0: error: -[FairExpoTests.AppConnectTests testAppConnect] : failed: caught error: "Failed to decode response:
{
  "data" : {
    "type" : "bundleIds",
    "id" : "…",
    "attributes" : {
      "name" : "…",
      "identifier" : "…",
      "platform" : "UNIVERSAL",
      "seedId" : "…"
    },
    "relationships" : {
      "bundleIdCapabilities" : {
        "meta" : {
          "paging" : {
            "total" : 0,
            "limit" : 2147483647
          }
        },
        "data" : [ {
          "type" : "bundleIdCapabilities",
          "id" : "…_IN_APP_PURCHASE"
        } ],
        "links" : {
          "self" : "https://api.appstoreconnect.apple.com/v1/bundleIds/…/relationships/bundleIdCapabilities",
          "related" : "https://api.appstoreconnect.apple.com/v1/bundleIds/…/bundleIdCapabilities"
        }
      },
      "profiles" : {
        "meta" : {
          "paging" : {
            "total" : 0,
            "limit" : 2147483647
          }
        },
        "data" : [ ],
        "links" : {
          "self" : "https://api.appstoreconnect.apple.com/v1/bundleIds/…/relationships/profiles",
          "related" : "https://api.appstoreconnect.apple.com/v1/bundleIds/…/profiles"
        }
      }
    },
    "links" : {
      "self" : "https://api.appstoreconnect.apple.com/v1/bundleIds/…"
    }
  },
  "links" : {
    "self" : "https://api.appstoreconnect.apple.com/v1/bundleIds"
  }
}
Error: dataCorrupted(Swift.DecodingError.Context(codingPath: [CodingKeys(stringValue: "data", intValue: nil), CodingKeys(stringValue: "attributes", intValue: nil), CodingKeys(stringValue: "platform", intValue: nil)], debugDescription: "Cannot initialize BundleIDPlatform from invalid String value UNIVERSAL", underlyingError: nil))."

@marcprux
Copy link
Contributor Author

marcprux commented Aug 9, 2022

I've filed a report with Apple about this discrepancy: FB11170909

@SwiftLeeBot
Copy link
Collaborator

Fails
🚫 Testing cancelled because the build failed.
🚫 Build service could not create build operation: internal error: could not generate PIF because the workspace has not finished loading or is still waiting for package resolution
Messages
📖

View more details on Bitrise

Code Coverage Report

Name Coverage

Generated by 🚫 Danger Swift against b337268

Copy link
Owner

@AvdLee AvdLee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dang, this is unfortunate! What we can do for now is to manually change the spec as well. We can also add a note to the readme that this is one of the manual steps to perform.

E.g, adding this to the readme:

Updating to a new spec file

  1. Download the spec
  2. Commit the spec to the project
  3. Ensure the following manual adjustments are done
    1. Add UNIVERSAL case to BundleIDPlatform

While I agree this is not ideal, I think this will do for now. Hopefully, Apple will fix this in the specs. Would you mind adding this to your PR?

@MortenGregersen
Copy link
Contributor

I've filed a report with Apple about this discrepancy: FB11170909

I filed a feedback in January last year (FB8977648) about the same 😬

@marcprux
Copy link
Contributor Author

marcprux commented Aug 11, 2022

Dang, this is unfortunate! What we can do for now is to manually change the spec as well. We can also add a note to the readme that this is one of the manual steps to perform.

I prefer automation. How about something like this:

curl -fsSL -o - https://developer.apple.com/sample-code/app-store-connect/app-store-connect-openapi-specification.zip | bsdtar -xOf - | jq '.components.schemas.BundleIdPlatform.enum |= [ "IOS", "MAC_OS", "UNIVERSAL" ]' > Sources/OpenAPI/app_store_connect_api_2.0_openapi_patched.json

A nice feature of running this command is that you could also add it as a periodic GitHub action that will checkout the version of the spec in HEAD and compare it with the latest version published on developer.apple.com, and if they are different, send a notification (or just fail the action). This will enable the project to quickly become aware of any new changes to the published spec.

I would be happy to help setup the workflow and add it to the PR. 👍🏼 if that sounds like a good idea.

@liamnichols
Copy link
Contributor

This is something that CreateAPI/CreateAPI#130 hopefully can help with (it's not implemented yet).

The proposed solution is to allow you to customise how some enums are generated and in this instance, you would be able to instead mark this one to be generated as a RawRepresentable struct that you could then expand with the undocumented constants. It would allow for a configuration like this:

enums:
  defaultType: enum
  typeOverrides:
    BundleIdPlatform: struct

To then generate something like this:

public struct BundleIDPlatform: RawRepresentable, Codable {
    public let rawValue: String

    public init(rawValue: String) {
        self.rawValue = rawValue
    }
    
    public static let iOS = Self(rawValue: "IOS")
    
    public static let macOS = Self(rawValue: "MAC_OS")
}

Which you could then extend in your own code:

extension BundleIDPlatform {
    public static let universal = Self(rawValue: "UNIVERSAL")
}

I don't have an ETA on this becoming available though I'm afraid but I just wanted to share a heads up incase it influences what you decide to do in the interim 👍

@AvdLee
Copy link
Owner

AvdLee commented Aug 24, 2022

I would be happy to help setup the workflow and add it to the PR. 👍🏼 if that sounds like a good idea.

@marcprux that would be amazing! Keeping this SDK up to date automatically that way would be such a big win. Please, feel free to open that PR!

This is something that CreateAPI/CreateAPI#130 hopefully can help with (it's not implemented yet).

@liamnichols my preference would be to solve it with Swift or the existing config, since it's more maintainable. So I'm definitely keen to adopt that new feature eventually!

@marcprux
Copy link
Contributor Author

@marcprux that would be amazing! Keeping this SDK up to date automatically that way would be such a big win. Please, feel free to open that PR!

I've submitted a PR adding a workflow to perform this check daily: #201. It isn't complete automation, but it is a good start.

@marcprux
Copy link
Contributor Author

I've submitted a PR adding a workflow to perform this check daily: #201. It isn't complete automation, but it is a good start.

You'll see that the check (https://github.com/AvdLee/appstoreconnect-swift-sdk/actions) is failing due to differences in the way jq formats the JSON and the way the current JSON is formatted. If you were willing to update the spec (which should be semantically identical) and manually commit these changes, then that should make the daily check start passing (and will also take care of #197).

@marcprux marcprux closed this by deleting the head repository Aug 30, 2022
@AvdLee
Copy link
Owner

AvdLee commented Sep 15, 2022

If you were willing to update the spec (which should be semantically identical) and manually commit these changes, then that should make the daily check start passing (and will also take care of #197).

What exactly do I have to update to the specs?

@marcprux
Copy link
Contributor Author

What exactly do I have to update to the specs?

Just run the script from the workflow (https://github.com/AvdLee/appstoreconnect-swift-sdk/blob/master/.github/workflows/sync_asc_api.yml) manually and then commit the changes:

curl -fsSL -o - https://developer.apple.com/sample-code/app-store-connect/app-store-connect-openapi-specification.zip | bsdtar -xOf - | jq '.components.schemas.BundleIdPlatform.enum |= [ "IOS", "MAC_OS", "UNIVERSAL" ]' > Sources/OpenAPI/app_store_connect_api_2.0_openapi.json

The next time the workflow runs, there won't be any diffs, and so it won't fail. And so any subsequent failures will indicate that the contents of the spec have truly changed, and thereby provide notification that a new release of appstoreconnect-swift-sdk should be created reflecting the changes in the API.

@AvdLee AvdLee mentioned this pull request Sep 19, 2022
@AvdLee
Copy link
Owner

AvdLee commented Sep 19, 2022

Perfect, that helped! Just merged the PR: #204
Let's see whether the workflow will succeed now 🙏

@marcprux
Copy link
Contributor Author

Bazinga!

@AvdLee AvdLee mentioned this pull request Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants