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

[normalize feature req] validation and safety utils #102

Closed
micimize opened this issue Oct 13, 2020 · 6 comments
Closed

[normalize feature req] validation and safety utils #102

micimize opened this issue Oct 13, 2020 · 6 comments

Comments

@micimize
Copy link
Contributor

I'm looking at adding some handlers in graphql/client.dart to check for / distinguish between malformed response data and cache misconfigurations, and I'm wondering if there's a better way than what I'm currently thinking.

Current thinking:

  • denormalizeOperation already validates all subtree structure right? So we can create a thin wrapper validateResponseStructure that ignores denormalization, or extract the traversal logic into its own helper
  • On response, we do a writeQuery/readQuery. If it returns null, we then:
    • throw a MismatchedDataStructureException if validateResponseStructure is null
    • throw a CacheRoundTripException otherwise (could be broken down further later).

The main idea is to validateResponseStructure independent of the cache. Might be a util better suited for gql.

@smkhalsa
Copy link
Member

I think this would be a great addition.

Would validateResponseStructure just return a bool? Or would it give some hints as to where the mismatched data is?

If a bool is sufficient, we could integrate validation into normalizeNode and denormalizeNode by default. Then we wouldn't need a separate validation method. We could just wrap writeQuery and readQuery in a try / catch and catch validation exceptions.

@micimize
Copy link
Contributor Author

Just throwing an exception is sufficient for now – eventually surfacing mismatched paths could be a goal but we can add that to the exception details later as well

@smkhalsa
Copy link
Member

smkhalsa commented Oct 13, 2020

After thinking about this further, without having access to the schema, we'd really only be able to validate whether the data should be a Map (i.e. the node has a SelectionSet) or a scalar (i.e. the node doesn't have a SelectionSet), and I'm not sure how useful this would be.

So we could either:

  1. include the schema as an optional parameter to the normalize functions, and if one is provided, perform a more thorough validation
  2. implement validation as a separate function

How do you envision this being used? Would it be called directly by the user? Or are you incorporating validation into the graphql client automatically?

@micimize
Copy link
Contributor Author

@smkhalsa we can tell from the document which fields the Map should have right? Because denormalize has to select the fields. That's mainly what I'm after, is just to validate that the structure is valid given the operation document.

This came up because for whatever reason a user's result.data was unexpectedly null and they had no idea why. So like, if a server responds to { foo, bar } with { foo: 1 }, denormalizeOperation(document, data) will return null

@smkhalsa
Copy link
Member

@micimize ah, ok.

The way that the denormalize functions work is that if a Map doesn't contain a key for a given FieldNode in the document (e.g. "bar" in your example above), the denormalizeNode function throws a PartialDataException. If returnPartialData is set to true, the exception is caught and ignored and the partial data is returned. If returnPartialData is set to false, the exception is caught and null is returned for the entire operation.

By default returnPartialData is set to false since:

  1. non-nullable fields could potentially return null data
  2. we may want to fetch from the network if all the data is not available for the operation (e.g. FetchPolicy.CacheFirst)

It sounds like in the case you describe above, if they want to receive partial data, they should be able to just set returnPartialData to true.

@smkhalsa
Copy link
Member

smkhalsa commented Nov 8, 2020

closed by #104

@smkhalsa smkhalsa closed this as completed Nov 8, 2020
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

No branches or pull requests

2 participants