paypal / mocca

Mocca is a GraphQL client for JVM languages with the goal of being easy to use, flexible and modular.
MIT License
15 stars 14 forks source link

Enhance `SelectionSet` annotation to ignore fields in the return type #27

Closed fabiocarvalho777 closed 2 years ago

fabiocarvalho777 commented 3 years ago

Add a new use case for SelectionSet annotation where users can still let Mocca resolve the selection set automatically, using the response DTO declared in the method return type, however, ignoring fields specified in a new attribute, called ignore, to be added to SelectionSet.

SelectionSet would then behave like this:

  1. If annotation is present and its value attribute is set, Mocca automatic selection set resolution is turned off, and SelectionSet value is used to define the selection set. In this case if ignore value is also set, then that is not used by Mocca, and a warning is logged.
  2. If annotation is present, its value attribute is NOT set, but ignore is, then Mocca automatic selection set resolution is turned ON, and SelectionSet ignore is used to pick which response DTO fields to ignore from the selection set.
  3. If annotation is present and both value and ignore attributes are NOT set, then a MoccaException is thrown.

Important Note

The fields marked to be ignored must NOT be present in the selection set defined in the request message! This might sound obvious but it is important to emphasize it to make sure the code changes pushed for this issue don't remove those fields only after the server already returned a response with them.

Acceptance Criteria

  1. Use case described above is implemented
  2. End user document is changed accordingly
  3. Javadocs are changed accordingly
  4. Unit and/or functional tests are added
  5. Make sure to add negative tests as well
AmandeepSingh97 commented 3 years ago

@fabiocarvalho777 Can you specify how the user will specify ignore for inner values?

For eg: if response type is {booleanVar complexField {innerBooleanVar innerIntVar innerStringVar} intVar stringVar} How will I specify ignore for innerBooleanVar and innerStringVar Also are we supporting to ignore the inner class itself as a whole?

For eg can we specify ignore to be complexField somehow which should ignore all its inner values? I need clarity on some of these cases.

fabiocarvalho777 commented 3 years ago
{
  booleanVar
  complexField {
    innerBooleanVar
    innerIntVar
    innerStringVar
  }
  intVar
  stringVar
}

Given the response type above, to ignore innerBooleanVar and innerStringVar, ignore should be set to {"complexField.innerBooleanVar", "complexField.innerStringVar"}. Please, as I said before, take a look at how ignore works and is documented for Var annotation. The format here is identical to that.

Also are we supporting to ignore the inner class itself as a whole?

Yes, we are. If we want to ignore complexField, all we have to do is set ignore to "complexField".

Let me know if you have other questions. Thanks.

AmandeepSingh97 commented 3 years ago

Thanks for clarifying @fabiocarvalho777

crankydillo commented 2 years ago
3. If annotation is present and both `value` and `ignore` attributes are **NOT** set, then a `MoccaException` is thrown.

Why don't we simply not allow this instead of exposing our users to runtime exceptions? Is a different annotation design (e.g. 2 different annotations) not an option? I don't know that much about 'selection sets', so forgive my ignorance.. Just trying to help with the PR review.

fabiocarvalho777 commented 2 years ago
3. If annotation is present and both `value` and `ignore` attributes are **NOT** set, then a `MoccaException` is thrown.

Why don't we simply not allow this instead of exposing our users to runtime exceptions? Is a different annotation design (e.g. 2 different annotations) not an option? I don't know that much about 'selection sets', so forgive my ignorance.. Just trying to help with the PR review.

Having two different annotations could be an option. There are some cons in this approach too. For example, what if the user uses both annotations at the same time? Wouldn't we have to check and prevent that in runtime anyways? How would we name both annotations? Could it be confusing for users to have two instead of one? Could it make the logic that process the annotations more complex?

So, when I think about the pros and cons, I think having one annotation sounds better, although not ideal.

What do you think?

crankydillo commented 2 years ago

I definitely think we can run with this as-is and let users complain.

Of your questions, the first one gives me the most pause. I am assuming we could come up with good names, but that worries me a bit as well. Honestly, I was thinking more about types (classes) when I made those comments. I haven't built many things where I leveraged annotations. Anyhow, if we ever revisit, here are some options that could be considered:

Again, I don't think this is required. I have a mild itch to investigate, but sadly time will likely not permit it.