fix: Removing newline characters from operation id hashes #3163
Conversation
- Updating operation/fragment definition handling to remove newline characters
✅ Deploy Preview for apollo-ios-docs canceled.
|
| @@ -14,7 +14,7 @@ struct OperationManifestItem { | |||
|
|
|||
| var source = operation.definition.source.convertedToSingleLine() | |||
| for fragment in operation.referencedFragments { | |||
| source += #"\n\#(fragment.definition.source.convertedToSingleLine())"# | |||
| source += #" \#(fragment.definition.source.convertedToSingleLine())"# | |||
There was a problem hiding this comment.
The recent change here was to make this a raw string with the # character for Swift. I think the root issue here is that because it is now a raw string the hash computation sees the raw two characters instead of when it was a regular string it would have been escaped down to an actual newline (single) character.
I don't know if it's 100% correct here to be replacing this with a string unless that's what we'll actually be sending - given that we're modifying the source it looks like it will be. Question then - does this break any backwards compatibility with older generated fragments or do we not need to care about that?
There was a problem hiding this comment.
That's a good point, the raw string could be contributing to the issue here. I would lean towards still removing the newline character as I don't remember there being any specific reason to have it other than we thought we should keep it before each fragment. This route would bring us inline with how Kotlin is handling this as well.
As for backwards compatibility I don't think this should be an issue because the original change was very recent for persisted queries and with this bug operations containing fragments aren't working correctly to begin with.
@AnthonyMDev thoughts?
There was a problem hiding this comment.
I think for the new persisted queries, we should be changing to this in order to align with Kotlin. But for the legacy APQs, this is going to break backwards compatibility with existing registered APQs...
The bigger issue is that we can't just change this here. We need to also change the formatting of the operation document that is actually sent when we do a network request. And if we do that, we'll need it to format it differently for APQs vs new PQs... that's where this can start to get messy and really error prone.
I'm not sure how to address this well without breaking existing APQs...
| @@ -14,7 +14,7 @@ struct OperationManifestItem { | |||
|
|
|||
| var source = operation.definition.source.convertedToSingleLine() | |||
| for fragment in operation.referencedFragments { | |||
| source += #"\n\#(fragment.definition.source.convertedToSingleLine())"# | |||
| source += #" \#(fragment.definition.source.convertedToSingleLine())"# | |||
There was a problem hiding this comment.
I think for the new persisted queries, we should be changing to this in order to align with Kotlin. But for the legacy APQs, this is going to break backwards compatibility with existing registered APQs...
The bigger issue is that we can't just change this here. We need to also change the formatting of the operation document that is actually sent when we do a network request. And if we do that, we'll need it to format it differently for APQs vs new PQs... that's where this can start to get messy and really error prone.
I'm not sure how to address this well without breaking existing APQs...
| @@ -169,7 +169,7 @@ class IR { | |||
|
|
|||
| var newline: String | |||
| for fragment in referencedFragments { | |||
| newline = #"\n"# | |||
| newline = "\n" | |||
There was a problem hiding this comment.
Ok so did a little bit of testing (thanks @calvincestari for calling out the raw string potentially being an issue) and believe the issue is the operation string in the json that is getting processed with the \n treated as an actual newline, but because we switched this piece of code in hash generation to a raw string it was being processed as the actual characters. After discussion with @AnthonyMDev this should fix the actual issue while maintaining backward compatibility for legacy apqs
Closes #3162