-
Notifications
You must be signed in to change notification settings - Fork 12
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
SLING-12014 - GraphQL Core needs to support additional fields from GraphQL Java #37
Conversation
graphql-java’s Selected Field
|
||
/** | ||
* @return the alias of the selected field or null if not alias was used | ||
*/ | ||
String getAlias(); | ||
|
||
/** | ||
* The result key is either the field query alias OR the field name in that preference order | ||
* | ||
* @return the result key of the selected field | ||
*/ | ||
String getResultKey(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add the JSR-305 annotations to all the methods? (return values and parameters)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is Jetbrains null annotations nowadays (https://sling.apache.org/documentation/development/null-analysis.html) :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I cannot because the source of the values does not provide any annotations or any other indication of the expectation. I will mark the all Nullable for now
public int getLevel() { | ||
return 0; | ||
} | ||
|
||
@Override | ||
public String getAlias() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public String getResultKey() { | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you return the actual values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed that - DONE
marked the new fields as nullable in the interface
I added the JCR-305 to all fields, methods and parameters. Right now all of them are Nullable that come from Graphql-java because they do not provide any indication there. |
with duplicate sub fields by simple name. There is still the expectation that fields are unique by FQN. Also added a test for the Selected Field for better coverage.
parameter order
Kudos, SonarCloud Quality Gate passed! |
I replaced the HashMap for fields by their simple name with a multi value map to avoid loosing fields when we have multiple fields with the same simple name. |
graphql-java’s Selected Field