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

Generate Swift operation IDs and operation <--> operation ID mapping file #147

Merged
merged 19 commits into from Jun 29, 2017

Conversation

kompfner
Copy link
Contributor

@kompfner kompfner commented Jun 22, 2017

This introduces - to Swift only - the notion of operationIds, which are unique identifiers for operations. The primary use for these ids is registration with a server, to do query persistence and whitelisting.

The operation id is generated by concatenating the formatting-normalized source of the operation with the formatting-normalized sorted sources of the fragments it references. It's a whitespace-agnostic "fingerprint" of the operation source that's sent to the server. Note, however, that it is not agnostic to field order.

When the operation-ids-path option is provided to apollo-codegen, the following occurs:

  • An operationId static property is added to the operation class definition.
  • The operation id and concatenated operation/fragment source blob are added to a map file.

The updated operation class definitions looks like:

public final class HeroAndFriendsQuery: GraphQLQuery {
  public static let operationString =
    "query HeroAndFriends($episode: Episode) {" +
    "  hero(episode: $episode) {" +
    "    __typename" +
    "    ...heroDetails" +
    "  }" +
    "}"

  public static let operationId = "6fecbcfcc753ec6f3f75c935270adeb5a7767397d783f7a747021cc86a9d59b7"
 
  // ...
}

The generated map file looks like:

{
  "6fecbcfcc753ec6f3f75c935270adeb5a7767397d783f7a747021cc86a9d59b7": {
    "name": "HeroAndFriends",
    "source": "query HeroAndFriends($episode: Episode) {\n  hero(episode: $episode) {\n    __typename\n    ...heroDetails\n  }\n}\nfragment heroDetails on Character {\n  __typename\n  name\n  ... on Droid {\n    __typename\n    primaryFunction\n  }\n  ... on Human {\n    __typename\n    height\n  }\n}"
  },
  "fe3f21394eb861aa515c4d582e645469045793c9cbbeca4b5d4ce4d7dd617556": {
    "name": "HeroAndFriendsNames",
    "source": "query HeroAndFriendsNames($episode: Episode) {\n  hero(episode: $episode) {\n    __typename\n    name\n    friends {\n      __typename\n      name\n    }\n  }\n}"
  },
  "bfaf75d346817b596a648fe6577f2068cdf95b7118cbed245f79720e82fafa9e": {
    "name": "HeroAppearsIn",
    "source": "query HeroAppearsIn {\n  hero {\n    __typename\n    name\n    appearsIn\n  }\n}"
  },
  "08f9b4c5c02d0f28f2fb9409286362f394b2195aeffd6ded783b9f6dcce7dd6f": {
    "name": "HeroName",
    "source": "query HeroName {\n  hero {\n    __typename\n    name\n  }\n}"
  },
  "b868fa9c48f19b8151c08c09f46831e3b9cd09f5c617d328647de785244b52bb": {
    "name": "TwoHeroes",
    "source": "query TwoHeroes {\n  r2: hero {\n    __typename\n    name\n  }\n  luke: hero(episode: EMPIRE) {\n    __typename\n    name\n  }\n}"
  }
}

Why not use persistgraphql?

  • I didn't want to introduce a dependency, and the approach taken seemed simpler
  • A lot of what persistgraphql concerns itself with is parsing and extraction, which apollo-codegen doesn't need (since it already does)
  • persistgraphql assigns an incrementing id to each operation; we want more of a unique "fingerprint"

Remaining work

  • Discuss & refine
  • Verify in apollo-ios repo (generate new API.swift with operation ids and verify all unit tests pass)
  • Add unit tests

@stubailo
Copy link
Contributor

I think this is great, since we should unify these tools anyway. No reason to have two IMO.

@martijnwalraven
Copy link
Contributor

Yeah, I think adding support for persisted queries to Apollo iOS is great! I want to make sure we align the approaches and tooling for the different platforms though.

As @stubailo mentions, we've discussed combining tools like apollo-codegen and persistgraphql before, and I think this makes a lot of sense. @rricard is also interested in working on this and even wrote a doc about unifying Apollo tooling.

It also seems there is an existing issue on persistgraphql that also calls for using a hash instead of an incrementing id.

@kompfner
Copy link
Contributor Author

@stubailo one reason I can think of for maintaining a standalone tool like persistgraphql is to cater to folks who aren't using Apollo. But you could make the counterargument "who's writing a bunch of .graphql files in their project and not using Apollo?"

@kompfner
Copy link
Contributor Author

Updated based on some helpful feedback by @lelandrichardson. Though operation names are guaranteed to be unique when running apollo-codegen, for the purposes of persisting operations on a server (which may be handling queries from multiple sources) it's not helpful to think of the operation name as the "key": it should be the operation id. Hence, I made the id the key in the output file.

@kompfner kompfner changed the title [WIP; OPEN FOR DISCUSSION] Generate Swift operation IDs and operation <--> operation ID mapping file Generate Swift operation IDs and operation <--> operation ID mapping file Jun 23, 2017
@martijnwalraven
Copy link
Contributor

Maybe this PR could be part of a larger effort to standardize the usage of persisted queries (or persisted documents as some are calling it apparently).

Basically, I'd like to avoid a situation where Apollo iOS does one thing, and other tools like persistgraphql do something entirely different.

One area in particular where I think standardization would be helpful is in the generated mapping file. Ideally, these mapping files would be supported by different servers and tools in the same way the extracted_queries.json file that persistgraphql generates is currently supported by Scaphold for example.

So I'm not saying we should stick to what persistgraphql currently does, but I'd at least like us to synchronize the changes (like switching to hashes or changing the mapping format).

@lewisf lewisf added the swift label Jun 26, 2017
@@ -376,6 +384,29 @@ export function structDeclarationForSelectionSet(
});
}

function operationId(generator, { operationName, fragmentsReferenced, source }, fragments) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be moved to compiler.js and put in the IR file, so that Android can have access to the same id.

@kompfner
Copy link
Contributor Author

Huh, I didn’t know about persistgraphql’s integration with the Scaphold platform. Very cool!

I agree with you, @martijnwalraven, that there’s value in standardizing the format of the generated mapping file, for use with different backend platforms. I think that there are some good reasons to evolve this format from the one that’s currently output by persistgraphql and imported by Scaphold to the format proposed here:

  • It seems like (for the server at least) the mapping from id —> operation text is more useful than the reverse mapping, suggesting that id should be the key.
  • Typically, keys are short unique identifiers, and they map to values that contain more information, not less, again suggesting that id should be the key.
  • In this PR we already have two pieces of information in each value (operation name and text), which our servers are prepared to consume at registration time. You can imagine other fields coming in the future: perhaps comments describing the operations, for display in a dashboard listing all persisted operations. This suggests that the value in the mapping should be a JSON object that can be extended in the future without breaking parsing older fields.

All that is to say - I propose we go with something like the format proposed in this PR. Perhaps we could support a command-line flag to output the format expected by Scaphold? What do you think?

@kompfner
Copy link
Contributor Author

kompfner commented Jun 28, 2017

@BenSchwab per your suggestion, moved the source-with-fragments and operation id generation to the compilation phase. Let me know if that will make it reusable on Android! Also added support for deeply-nested fragment references, which thanks to you we realized was missing.

Paul Kompfner added 16 commits June 28, 2017 10:02
…anteed by apollo-codegen to be unique) and providing values with both operation id and source
…compilation step rather than code generation step.

Also, when generating operation source + fragments, handle possibility of nested fragment references.
… ids and sources on context (it's no longer done that way)
…ted fragment refrences, the correct source + fragments is generated for the operation id mapping file
@@ -69,7 +69,7 @@ export function generateSource(context, options) {
});

Object.values(context.operations).forEach(operation => {
classDeclarationForOperation(generator, operation);
classDeclarationForOperation(generator, operation, Object.values(context.fragments));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still needed? Or is sourceWithFragments enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I think you're right that we don't need this anymore.

}

generator.printNewlineIfNeeded();
generator.printOnNewline(`public static let operationId = "${operationId}"`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely a bit nitpicky, but would operationID or even operationIdentifier be more Swifty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, yeah, you're right. Let's go with operationIdentifier instead (I'm looking at URLSessionTask.taskIdentifier as inspiration).

namespace: argv.namespace
namespace: argv.namespace,
operationIdsPath: argv["operation-ids-path"],
generateOperationIds: !!argv["operation-ids-path"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be a separate option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do think having this as a separate option reads a lot better, especially in codeGeneration.js, which doesn't need to know or care about file paths.

Paul Kompfner added 2 commits June 29, 2017 11:09
- Improve CLI message
- Remove unnecessary parameter in a function call
- Rename Swift operationId to operationIdentifier
@kompfner kompfner merged commit 703edf2 into apollographql:master Jun 29, 2017
@kompfner kompfner deleted the operation-ids branch June 29, 2017 18:32
@kompfner kompfner mentioned this pull request Jul 5, 2017
ecstasy2 added a commit to ecstasy2/apollo-codegen that referenced this pull request Dec 22, 2017
…file (apollographql#147)

* Initial progress on Swift operationId generation

* Add dependency on crypto library for SHA256

* In Swift, operationId is computed and output with every operation class

* Factor out Swift operationId into its own function

* Write to a JSON file the mapping between operations and their ids (Swift-only)

* Update operationIds map to be keyed by operation name (which are guaranteed by apollo-codegen to be unique) and providing values with both operation id and source

* OperationIds map is keyed by id, and operation name is part of the value

* Use === instead of ==

* Update Swift Jest snapshots with new newline

* Fix missing comma

* Add fragments to classDeclarationForOperation invocation in Swift tests

* Add unit tests exercising operation id generation in Swift

* Move generation of operation IDs and operation source + fragments to compilation step rather than code generation step.
Also, when generating operation source + fragments, handle possibility of nested fragment references.

* Remove obsolete test checking for generated mapping between operation ids and sources on context (it's no longer done that way)

* Improve readability of generateOperationIds option in compileFromSource function in Swif tests

* Add Swift code generation test that verifies that, when there are nested fragment refrences, the correct source + fragments is generated for the operation id mapping file

* Fix typescript error related to operation ids map

* PR feedback and tweaks:
- Improve CLI message
- Remove unnecessary parameter in a function call
- Rename Swift operationId to operationIdentifier

* Update test snapshot
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.

None yet

5 participants