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

High likelihood of identical hash values for GraphQLOperation types #3467

Closed
aim2120 opened this issue Oct 31, 2024 · 4 comments · Fixed by apollographql/apollo-ios-dev#530
Closed
Labels
question Issues that have a question which should be addressed

Comments

@aim2120
Copy link

aim2120 commented Oct 31, 2024

Question

I recently discovered that GraphQLOperation hashing does not differentiate operations as much as I thought it would. In fact, if two operations both have no variables, they have the same hash value, even if they're entirely different operation types.

This is due to the way hash(into:) is implemented for GraphQLOperation

public extension GraphQLOperation {
  var __variables: Variables? {
    return nil
  }

  func hash(into hasher: inout Hasher) {
    hasher.combine(__variables?._jsonEncodableValue?._jsonValue)
  }
}

This seems like an unfortunate design decision. A few questions for the maintainers of the project:

  • Is there a particular reasons why there's a high likelihood completely different operations may have the same hash value?
  • Would it be viable to change the hash implementation to hash on operation identifiers in addition to the variables (e.g. operation name, ID, type)?

I've already updated my usage of the hash value based on this finding, but wanted to create an issue in case it may affect others.

@aim2120 aim2120 added the question Issues that have a question which should be addressed label Oct 31, 2024
@calvincestari
Copy link
Member

Thanks for raising the issue @aim2120. Are you using the hash value from an operation directly that you noticed this?

Copy link
Contributor

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo iOS usage and allow us to serve you better.

@calvincestari
Copy link
Member

@aim2120, the change is merged and will go out in the next release.

@aim2120
Copy link
Author

aim2120 commented Nov 7, 2024

Hey @calvincestari, thanks for getting that updated! Yes, I was using the hash value in an interceptor implementation when I noticed the behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Issues that have a question which should be addressed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants