apollographql / apollo-tooling

✏️ Apollo CLI for client tooling (Mostly replaced by Rover)
https://apollographql.com
MIT License
3.04k stars 469 forks source link

[Swift] Codegen for input types has non-deterministic order #1599

Open AttilaTheFun opened 5 years ago

AttilaTheFun commented 5 years ago

I've been extremely frustrated with the fact that the order of codegen'd initializer parameters for input objects appears to be nondeterministic. I believe the order for regular fragments is the order of the fields in the query, but for input objects, there is no obvious order. E.g.:

mutation CreateSession($request: CreateSessionRequest!) {
  createSession(request: $request) {
    session {
      ...allSessionFields
    }
  }
}

If CreateSessionRequest has two arguments, say a userID and a token, sometimes I get:

CreateSessionRequest(userID:token:) and other times I get CreateSessionRequest(token:userID)

Every time I update my schema, and refetch the schema from my server using the CLI, then re-codegen from that schema, a bunch of parameters for my requests change order.

The auto-fixits often only reorder the parameter names and not the values making them dangerous. This means that every time I update the schema, when I codegen I have to go fix how I call these initializers in many places. At least I'm able to shield some of my code from this liability by providing a consistent API which abstracts away Apollo using domain models, but it's still painful.

I've gone so far as writing a script to alphabetize the fetched schema (which helps reduce noise in commits), but the generated code still seems to be nondeterministic and the parameters do not seem to follow any discernible ordering.

Example:

In my alphabetized schema, you can see that the fields of EditableUserFields are in the correct order:

      {
        "description": "", 
        "enumValues": null, 
        "fields": null, 
        "inputFields": [
          {
            "defaultValue": null, 
            "description": "", 
            "name": "biography", 
            "type": {
              "kind": "SCALAR", 
              "name": "String", 
              "ofType": null
            }
          }, 
          {
            "defaultValue": null, 
            "description": "", 
            "name": "emailAddress", 
            "type": {
              "kind": "NON_NULL", 
              "name": null, 
              "ofType": {
                "kind": "SCALAR", 
                "name": "String", 
                "ofType": null
              }
            }
          }, 
          {
            "defaultValue": null, 
            "description": "", 
            "name": "fullName", 
            "type": {
              "kind": "NON_NULL", 
              "name": null, 
              "ofType": {
                "kind": "SCALAR", 
                "name": "String", 
                "ofType": null
              }
            }
          }, 
          {
            "defaultValue": null, 
            "description": "", 
            "name": "profilePictureURL", 
            "type": {
              "kind": "NON_NULL", 
              "name": null, 
              "ofType": {
                "kind": "SCALAR", 
                "name": "String", 
                "ofType": null
              }
            }
          }, 
          {
            "defaultValue": null, 
            "description": "", 
            "name": "username", 
            "type": {
              "kind": "NON_NULL", 
              "name": null, 
              "ofType": {
                "kind": "SCALAR", 
                "name": "String", 
                "ofType": null
              }
            }
          }, 
          {
            "defaultValue": null, 
            "description": "", 
            "name": "website", 
            "type": {
              "kind": "SCALAR", 
              "name": "String", 
              "ofType": null
            }
          }
        ], 
        "interfaces": null, 
        "kind": "INPUT_OBJECT", 
        "name": "EditableUserFields", 
        "possibleTypes": null
      }, 

However in the generated code, I see this order:

EditableUserFields(biography:,profilePictureUrl:,fullName:,username:, emailAddress:, website:)

Is there a way to address this? Am I holding it wrong? Or do we actually need to fix it on the Apollo end? I'm surprised no one I could find was complaining about this as it seems like it would be a common bugbear for consumers of Apollo, making me think that there's something odd about my setup which is causing this noise.

designatednerd commented 5 years ago

@AttilaTheFun Can you confirm what version of the codegen and node you're using? If it's set up with the run-bundled-codegen script, that should print it out in the logs

AttilaTheFun commented 5 years ago

@designatednerd I'm using the latest bundled codegen:

Correct version of the CLI tarball is included, checking if it's already been extracted...
Current verson of CLI is already extracted!
Apollo CLI Information: apollo/2.19.1 darwin-x64 node-v10.16.3
+ /Users/logan/Documents/Source/ios/MyApp/DerivedData/MyApp/SourcePackages/checkouts/apollo-ios/scripts/apollo/bin/run codegen:generate --target=swift '--includes= MyApp/GraphQL/**/*.graphql' --localSchemaFile=MyApp/GraphQL/Schema/schema.json --passthroughCustomScalars API/Sources/API/API.swift
designatednerd commented 5 years ago

Yeah I would have expected this to cause issues previously as well - it looks from the output like you're running codegen against the bundled node, so i don't think it's a Node version issue, at least.

Looking at structDelcarationForInputObjectType, which generates that code, it seems to be grabbing an array of fields based on the input object type and then mapping that into the array of properties - it always keeps things in the same order it receives them in when it hits the swift-generating bit, so as far as I can tell something's causing the order to get screwy either at the Object.values(type.getFields()) call, or when the fields are being generated.

I'm not seeing anything terribly obvious in terms of what could cause the order of these things to change. And it sounds like re-codegening is happening on the same device? I've seen some damned weird behavior going from 32 to 64 bit laptops (or more often, servers to laptops), but it sounds like you're seeing inconsistency on the same machine.

I'll ping my JS crew on this, maybe they have some thoughts as well.

designatednerd commented 5 years ago

@AttilaTheFun Also, is this new behavior on 2.19.1, or do you recall when this behavior started?

AttilaTheFun commented 5 years ago

@designatednerd this has been happening to me for a long time. Fortunately, it seems smart enough to not regenerate the code if my schema or queries haven't changed. But every time I create or update a .graphql file, or pull the schema again, I have to fix a bunch of params for request objects. Also FWIW this only seems to really be an issue for input objects because you don't specify the fields to provide on them. E.g.:

let request = CreateSessionRequest(token: token, tokenId: tokenID)
let mutation = CreateSessionMutation(request: request)
return self.perform(mutation: mutation, queue: .main)
    .map { data in
        return Session(fields: data.createSession.session.fragments.allSessionFields)
    }

Token and tokenId will often switch positions after codegen. If I click the fixit in Xcode, it will often swap the params but not their values giving me:

let request = CreateSessionRequest(tokenId: token, token: tokenID)

Which would be a bug. This is really unfortunate - I would hope that the codegen would produce deterministic output :(

designatednerd commented 5 years ago

In theory it should be deterministic (and in theory we'd get a lot more complaints about this if it weren't) - it seems like somewhere lower down the stack things aren't being pulled out deterministically.

I'm curious - are you seeing this issue on multiple computers or just a single one? What setup are you using if it's a single one?

AttilaTheFun commented 5 years ago

I only have one computer right now so I'm not sure. I do think relying on any sort of order in the schema or queries could be a bit problematic, though. The schema doesn't have any real way of expressing order for params or fields.

I think in general it would be best to have Apollo alphabetize the parameters, at least for input objects. So if I had "id" and "token", "id" would always precede "token" since "I" is lexicographically before "t".

designatednerd commented 5 years ago

Yeah, I generally agree with this thought. I'll have to figure out some kind of way to test it though...

grepug commented 3 years ago

Any updates for this? I encountered this problem recently.

AttilaTheFun commented 3 years ago

@designatednerd I'm still encountering this issue as well. FWIW it only seems to happen for me for input objects. I.e. input parameters keep their order, but if one of those parameters is itself an object, the order of the fields is random.

designatednerd commented 3 years ago

Yeah this should be addressed as part of the new codegen described in the RFC document in Apollo-iOS #1876. We elected to focus our energy on making the new Codegen rather than improving the old one since the new one will be in Swift, which will make it easier for iOS developers to contribute to it.