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

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

Closed
fabiocarvalho777 opened this issue Aug 10, 2021 · 6 comments · Fixed by #39
Closed

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

fabiocarvalho777 opened this issue Aug 10, 2021 · 6 comments · Fixed by #39
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@fabiocarvalho777
Copy link
Member

fabiocarvalho777 commented Aug 10, 2021

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
Copy link
Contributor

@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
Copy link
Member Author

{
  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
Copy link
Contributor

Thanks for clarifying @fabiocarvalho777

@crankydillo
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

crankydillo commented Sep 11, 2021

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:

  • Single annotation that uses an enumeration (not sure on this, but I think I've seen it). Yep, this is what you get when I try to code at night!
  • Compile-time annotation processing (this could solve mutually exclusive annotation/property problem)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants