Deprecating queryStringLiteralFormat #3129

BobaFetters · 2023-07-19T16:58:39Z

-Deprecating the queryStringLiteralFormat config option for codegen
-All operation definitions will now be single line
-Updated operation manifest identifier creation to be based on single line definitions

- Updated the Operation Manifest generation to be able to do either an absolute path as provided (which is the current functionality) or to take relative paths that begin with './' and have the pathing be relative to the config rootURL -Updated tests to reflect these changes

-Deprecating the queryStringLiteralFormat config option for codegen -All operation definitions will now be single line -Updated operation manifest identifier creation to be based on single line definitions

glasser

I feel like I am still a bit confused here. The PQM generation code now hardcodes single-line. But while you may have deprecated the option, you still can select multi-line output. Why not make these two things match up?

Specifically, if you were to:

Select deprecated multi-line output
Register PQs (which are single-line)
Configure your client to still just send freeform GraphQL, not PQ IDs
Turn on safelisting in your Router, with "matching freeform still allowed" mode

then Router will block the freeform GraphQL because you registered them as single-line but are sending them as multi-line.

Perhaps this is an odd choice of configuration and the answer should be "just don't do that". But the design goal for the manifest is that it should be as close to the exact bytes sent at runtime as possible — I'm not sure why we would have a formatting option that is ignored when generating PQ manifests.

glasser · 2023-07-19T17:00:43Z

Sources/ApolloCodegenLib/FileGenerators/OperationManifestFileGenerator.swift

    for fragment in operation.referencedFragments {
-      source += "\n\(fragment.definition.source)"
+      source += "\\n\(fragment.definition.source.convertedToSingleLine())"


I don't know swift but that doubling of the backslash on the first line looks suspect to me — unless the source field is being used in a different way now? That said the tests don't seem to have changed in an odd way...

So the \\n will actually insert \n as characters in the string vs creating a newline. My understanding from discussions with @AnthonyMDev was that we want everything to be single line now, but we should still have a newline character before each fragment, so this change allows the overall string to be single line while still containing the newline characters before each fragment. If this isn't what we intend I'm happy to rework it, this is just what I gathered we wanted.

This particular change goes with Tests/ApolloCodegenTests/CodeGeneration/FileGenerators/OperationManifestFileGeneratorTests.swift right? I think it would be interesting to see how a test case that includes a fragment looks before and after the change in this PR. My suspicion is that you will be adding way too many backslashes. But I might be wrong!

Hmmm I think you may be right, looking at the updated expected test results I got from the code output it looks like it might have the extra slash \\n instead of just having \n like it should, definitely an oversight on my part when updating those test results. Thanks for the callout!

Looking into this further the actual output in the manifest file correctly contains the \n but for some reason the test result looks like maybe it is containing a \\n instead.

I guess my point is that it looks like there's a semantic change here so either:

(a) There's some other change in your PR that cancels it out and there's no end-to-end-change
(b) The old code was actually wrong and this is a bug fix
(c) The old code was right and this is a bug

I don't have enough context to know which is which though! (I don't even know what this particular file is used for, honestly.)

Ok looking into everything is actually correct, the expected test results do contain a \\n because of the need to escape the \ so when it gets compared to the actual output which only contains \n it succeeds as expected.

So to your point, this (a) where it is a specific change in this PR because everything is now supposed to be single line, the original \n would add a newline to the definition for each fragment, and the new setup with \\n inserts the actual characters \n into the definition string before each fragment.

So a sample out in a manfiest file from this code looks like this:

{ "format": "apollo-persisted-query-manifest", "version": 1, "operations": [ { "id": "4de9229ebd29faf64c0f3f88e61a77b23d7276fc6d24263b1e0378229ce29744", "body": "query GetThing { thing { __typename ...Name } }\nfragment Name on Thing { __typename name }", "name": "GetThing", "type": "query" }, ] }

This stuff is always really awkward in Swift. Thanks for the call out @glasser and for double checking your work @BobaFetters.

Huh. But a raw newline isn't actually legal JSON — if you're saying the old code did something more like

{ "format": "apollo-persisted-query-manifest", "version": 1, "operations": [ { "id": "4de9229ebd29faf64c0f3f88e61a77b23d7276fc6d24263b1e0378229ce29744", "body": "query GetThing { thing { __typename ...Name } } fragment Name on Thing { __typename name }", "name": "GetThing", "type": "query" }, ] }

then was it just broken?

That's a good point, technically for it to also be valid in the JSON it needs to be escaped as well so in the final JSON it should be \\n so the Swift side needs updated to actually put a \\n into the JSON string

As for how it used to work I'm not sure will have to go back and look at how things worked originally for APQs.

BobaFetters · 2023-07-19T17:37:37Z

So with the current changes you could still technically say "I want multi-line definitions" through the deprecated initializer but it won't be used. Everything will always be single line now, the reasoning for having things the way they are now is to not break users existing code which may use the now deprecated initializer and multi-line output, they will get deprecation warnings but their code would still compile. Then with the next major release we will completely remove the deprecated code.

glasser · 2023-07-19T18:00:21Z

So with the current changes you could still technically say "I want multi-line definitions" through the deprecated initializer but it won't be used. Everything will always be single line now, the reasoning for having things the way they are now is to not break users existing code which may use the now deprecated initializer and multi-line output, they will get deprecation warnings but their code would still compile. Then with the next major release we will completely remove the deprecated code.

To reiterate: the design of the PQ feature is that the string in the manifest should match the string sent at runtime. If there is an option that controls one but not the other, then that can cause problems downstream. Is it feasible to apply the preference when generating the manifest string too?

Alternatively, you could make it an error to explicitly specify "multi line strings" if you are generating the new PQM format.

But I just would really like to not have a mode where the runtime string and the manifest string are different if it is at all avoidable.

BobaFetters · 2023-07-19T18:19:41Z

So with these changes the runtime and manifest definition strings should be identical always because there is no longer an option to choose how they are formatted, they are just always done as single line strings, the ability to configure their format has been removed.

…ted-queries-identifiers

AnthonyMDev · 2023-07-19T19:22:28Z

To reword what @BobaFetters is saying, your concern is addressed by this PR @glasser. In the next release of Apollo iOS there will be no way to actually send the document as a multi-line string. We are removing the functionality completely. We are marking the old configuration option as deprecated so that users who are using it get a warning notifying them that the option is no longer available, rather than a fatal error when they run codegen due to an unexpected key in their JSON config.

We've realized this option was never really necessary in the first place. Using multiline string made some generated code more human readable at the expense of always being worse for performance and network traffic load. Apollo Kotlin was already doing single-line document's only. Removing this functionality isn't going to break anyone's code in any way, it just is going to change the formatting of the document so it's not as friendly for human-eyes.

The fact that this was adding additional complexity to PQs was what prompted us to make the decision, but we feel that it's otherwise an improvement anyways.

glasser · 2023-07-19T20:03:35Z

Ohhhhhhhh. So it's not deprecated in the sense that it logs a warning and still works -- just in the sense that it doesn't break you at compile time if you pass it?

AnthonyMDev

Thanks so much for this @BobaFetters!

Got a couple of very minor comments, but the biggest thing I'm concerned about is making sure we aren't missing some important reason why @calvincestari added the JSON encoding to the APQ Identifier template.

AnthonyMDev · 2023-07-19T19:26:04Z

Sources/ApolloCodegenLib/ApolloCodegenConfiguration.swift

+  )
+  public init(
+    additionalInflectionRules: [InflectionRule] = Default.additionalInflectionRules,
+    queryStringLiteralFormat: QueryStringLiteralFormat = .singleLine,


We should remove the default value for the parameter here. The only way you would be using this deprecated initializer is if you were explicitly providing that parameter. Removing the default value prevents any chances of an ambiguous initializer pattern selecting this deprecated initializer.

AnthonyMDev · 2023-07-19T19:26:54Z

Sources/ApolloCodegenLib/FileGenerators/OperationManifestFileGenerator.swift

    for fragment in operation.referencedFragments {
-      source += "\n\(fragment.definition.source)"
+      source += "\\n\(fragment.definition.source.convertedToSingleLine())"


This stuff is always really awkward in Swift. Thanks for the call out @glasser and for double checking your work @BobaFetters.

Sources/ApolloCodegenLib/Templates/FragmentTemplate.swift

AnthonyMDev · 2023-07-19T19:30:43Z

Sources/ApolloCodegenLib/Templates/LegacyAPQOperationManifestTemplate.swift

-  }
-
-  private func template(_ operations: [OperationManifestItem]) throws -> TemplateString {
-    let encoder = JSONEncoder()


Are you confident we should be removing this json encoding stuff? If I remember correctly, @calvincestari added this because something in the JSON wasn't formatting correctly otherwise, but I can't remember the details.

I guess if the tests are passing we might be alright, but we should check with Calvin.

So the JSON encoding was causing issues with us now trying to actually have a \n in our strings before a fragment, so the persisted query manifest would would fine because it manually creates it's JSON but this would fail tests because the encoding was removing our newline characters.

…ted-queries-identifiers

netlify · 2023-07-19T21:18:03Z

✅ Deploy Preview for apollo-ios-docs canceled.

Name	Link
🔨 Latest commit	`29eb738`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-ios-docs/deploys/64b99da4c5347f0008f0463b

Updating strings to be raw strings to ensure that the resulting JSON for operation manifest files contains properly escaped \\n in strings instead of \n which is not valid JSON

AnthonyMDev

I'm having a hard time understanding some of the specifics of the string formatting of the JSON. But it sounds like you have done your due diligence in ensuring this was the correct way to handle it.

AnthonyMDev · 2023-07-20T20:20:19Z

...loCodegenTests/CodeGeneration/Templates/PersistedQueriesOperationManifestTemplateTests.swift

      {
        "format": "apollo-persisted-query-manifest",
        "version": 1,
        "operations": [
          {
-            "id": "efc7785ac9768b2be96e061911b97c9c898df41561dda36d9435e94994910f67",
+            "id": "9db65faebf9e503b403964a81c90edbeeb894d46029b1b42b16639dda96772bd",
            "body": "query Friends { friends { ...Name } }\\nfragment Name on Friend { name }",


And we are certain that the \\n is the correct way to represent the newline character in the JSON? It seems odd to me, but I see that you and @glasser discussed this in detail.

The simplest thing might be to just run the thing outside of a test suite on a project with operations with fragments and share the output file here...

Agreed, that's definitely something that could be implemented in a lot of our testing that might make things easier

AnthonyMDev · 2023-07-20T20:20:28Z

Sources/ApolloCodegenLib/FileGenerators/OperationManifestFileGenerator.swift

@@ -14,7 +14,7 @@ struct OperationManifestItem {

    var source = operation.definition.source.convertedToSingleLine()
    for fragment in operation.referencedFragments {
-      source += "\\n\(fragment.definition.source.convertedToSingleLine())"
+      source += #"\\n\#(fragment.definition.source.convertedToSingleLine())"#


This looks odd to me, but if the file comes out formatted correctly then that's fine. It seems like you have unbalanced # and I thought string interpolation wouldn't work inside of a raw string.

After further discussion/investigation it looks like the proper way to include the newline character in JSON string is \n and not \\n

BobaFetters added 2 commits July 19, 2023 10:53

Deprecating queryStringLiteralFormat

ca85c0c

-Deprecating the queryStringLiteralFormat config option for codegen -All operation definitions will now be single line -Updated operation manifest identifier creation to be based on single line definitions

BobaFetters requested review from glasser, calvincestari and AnthonyMDev July 19, 2023 16:58

BobaFetters self-assigned this Jul 19, 2023

glasser reviewed Jul 19, 2023

View reviewed changes

Fixing fragment template tests

a43baad

BobaFetters added 2 commits July 19, 2023 14:52

Minor file extension check update

2a7d148

Merge branch 'feature/persisted-queries-manifest' into feature/persis…

01d7b3e

…ted-queries-identifiers

AnthonyMDev approved these changes Jul 19, 2023

View reviewed changes

BobaFetters added 2 commits July 19, 2023 16:40

Updating file extension handling and tests

5b5b678

Merge branch 'feature/persisted-queries-manifest' into feature/persis…

1838858

…ted-queries-identifiers

Base automatically changed from feature/persisted-queries-manifest to main July 19, 2023 20:55

Removing default value in initializer

40a01ed

Updating newline character handling

eb18619

Updating strings to be raw strings to ensure that the resulting JSON for operation manifest files contains properly escaped \\n in strings instead of \n which is not valid JSON

BobaFetters requested a review from AnthonyMDev July 20, 2023 17:26

AnthonyMDev approved these changes Jul 20, 2023

View reviewed changes

Reverting newline character change in JSON string

29eb738

After further discussion/investigation it looks like the proper way to include the newline character in JSON string is \n and not \\n

BobaFetters merged commit 874c7ff into main Jul 20, 2023
14 of 17 checks passed

BobaFetters deleted the feature/persisted-queries-identifiers branch July 20, 2023 20:52

AnthonyMDev pushed a commit that referenced this pull request Aug 2, 2023

Deprecating queryStringLiteralFormat (#3129)

d7d08d2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecating queryStringLiteralFormat #3129

Deprecating queryStringLiteralFormat #3129

BobaFetters commented Jul 19, 2023

glasser left a comment

glasser Jul 19, 2023

BobaFetters Jul 19, 2023

glasser Jul 19, 2023

BobaFetters Jul 19, 2023

BobaFetters Jul 19, 2023

glasser Jul 19, 2023

BobaFetters Jul 19, 2023

AnthonyMDev Jul 19, 2023

glasser Jul 19, 2023 •

edited

BobaFetters Jul 19, 2023

BobaFetters commented Jul 19, 2023

glasser commented Jul 19, 2023

BobaFetters commented Jul 19, 2023

AnthonyMDev commented Jul 19, 2023

glasser commented Jul 19, 2023

AnthonyMDev left a comment

AnthonyMDev Jul 19, 2023

AnthonyMDev Jul 19, 2023

AnthonyMDev Jul 19, 2023

BobaFetters Jul 19, 2023

netlify bot commented Jul 19, 2023 •

edited

AnthonyMDev left a comment

AnthonyMDev Jul 20, 2023

glasser Jul 20, 2023

BobaFetters Jul 20, 2023

AnthonyMDev Jul 20, 2023

Deprecating queryStringLiteralFormat #3129

Deprecating queryStringLiteralFormat #3129

Conversation

BobaFetters commented Jul 19, 2023

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser Jul 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BobaFetters commented Jul 19, 2023

glasser commented Jul 19, 2023

BobaFetters commented Jul 19, 2023

AnthonyMDev commented Jul 19, 2023

glasser commented Jul 19, 2023

AnthonyMDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Jul 19, 2023 • edited

✅ Deploy Preview for apollo-ios-docs canceled.

AnthonyMDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser Jul 19, 2023 •

edited

netlify bot commented Jul 19, 2023 •

edited