Open jeremygiberson opened 6 years ago
FYI I'm working on a PR for this suggestion
I like the idea of a operation class to distinguish that parameter separately from the fields, it could clean the code up a lot. But I think a good first step might be adding support for variables in the @GraphQLProperty
annotation as well as making @GraphQLArgument
, @GraphQLArguments
, @GraphQLVariable
, and @GraphQLVariables
work above classes as well and not just fields. It will also then be very simple to mimic most of that behavior in a new @GraphQLOperation
annotation down the road. This should actually be relatively simple and small change too since Argument and Variable classes are essentially the same and arguments are already supported in the property annotation, just different names to be explicit and clear.
Let me know your thoughts on this! Might be worth a separate PR for those changes first and then a revisit on #17 afterwards. I'll also try to keep discussion on the topic here and code specifics in the PR.
Hey Chemdrew,
My thoughts,
Concerning @GraphQLArgument
,@GraphQLArguments
I agree that these should available at class level. There is an edge scenario this will affect -- right now a class w/out property annotations is bypassed and it's properties become the top level property. For example TestModelSimple
in the tests, when passed by itself through the request builder becomes { simpleString }
as opposed to TestModelSimple { simpleString }
. So the question arises, what to do with a model w/ arguments but no property annotation. Can/should we require Arguments to be specified within @GraphQLProperty
? Or just treat the model as a top level property inferring name from class?
Regarding @GraphQLVariable
and @GraphQLVariables
and adding variables to @GraphQLProperty
-- I think the best direction for these annotations is to deprecate @GraphQLVariables
altogether and restrict @GraphQLVariable
usage to only in @GraphQLOperation
. A few justifications for this:
query myQuery($userId: String, $UserID: String, $UserId: String) { ....
Example. Variables defined by models
@GraphQLVariable(name = "UserID")
class EmailPreferences {
}
@GraphQLVariable(name = "userId")
class TextPreferences {
}
class PreferencesOperation {
public EmailPreferences emailPreferences;
public TextPreferences textPreferences;
}
// despite arguments having different names (case counts) we are able to share the (singular) userId variable with both of them
builder.setVariables(Variable(name="UserID", userIdValue));
builder.setVariables(Variable(name="userId", userIdValue));
Output
query PreferencesOperation($UserID: String, $userId: String) { EmailPreferences(UserID: $UserID) { ... } TextPreferences(userId: $userId) { ... } }
Vs Arguments defined by models
@GraphQLArgument(name = "UserID")
class EmailPreferences {
}
@GraphQLArgument(name = "userId")
class TextPreferences {
}
@GraphQLOperation(variables={ @GraphQLVariable(name = 'userId', scalar = 'String') })
class PreferencesOperation {
public EmailPreferences emailPreferences;
public TextPreferences textPreferences;
}
// despite arguments having different names (case counts) we are able to share the (singular) userId variable with both of them
builder.setArguments('emailPreferences', Argument(name="UserID", variable='userId'));
builder.setArguments('textPreferences', Argument(name="userId", variable='userId'));
Output
query PreferencesOperation($UserID: String) { EmailPreferences(UserID: $UserID) { ... } TextPreferences(userId: $UserID) { ... } }
So I think variables outside of the Operation annotation just facilitate some icky consequences. What do you think?
Making argument(s) class level is a small change that shouldn't have any impact on the operation code. So I can bring that into the PR easily.
However,
It will also then be very simple to mimic most of that behavior in a new @GraphQLOperation annotation down the road.
Do I understand this to be that you don't like how operation is implemented within this PR and are looking to do it differently pending changes to class level updates to the specified annotations?
So far my approach has been to treat the model as a top level property inferring name from class, and just using the property annotation to apply overrides there - which is what had me excited by the idea of having the operation class separate because then a lot of logic around which fields are present and conditionals on how to handle them can be simplified.
So for a first simple implementation I would picture it looking like
@GraphQLArguments({
@GraphQLArgument(name="first"),
@GraphQLArgument(name="second")
})
with the request being
query {
TestModelSimple(first: null, second: null) {
simpleString
}
}
In regards to variables, I agree there are some big consequences to using variables that require a significant understanding of the GraphQL specification but I believe allowing them on all fields provides a huge benefit that outweighs this. A specific example of this is for Date fields. For every date field we use a custom resolver that wraps moment.js to allow the consumers to specify timezone/format/locale of that date. Many of our models have multiple date fields in each and this is the perfect use-case for variables since different consumers may want a different timezone/format/locale but they will also want that consistently maintained throughout the response.
I like the way you have operations implemented currently, but I think adding the option of a variable
field in @GraphQLProperty
, and making @GraphQLArgument(s)
and @GraphQLVariable(s)
work above classes would all be beneficial in separate PRs just to break things up into smaller features to make things easier
Roger on arguments translation to TestSimpleModel.
A specific example of this is for Date fields. For every date field we use a custom resolver that wraps moment.js to allow the consumers to specify timezone/format/locale of that date. Many of our models have multiple date fields in each and this is the perfect use-case for variables since different consumers may want a different timezone/format/locale but they will also want that consistently maintained throughout the response.
If I understand your use case correctly, I believe operation-vars/property-arguments cover this scenario. .
The use case requirements as I understand: 1 consumer can specify the timezone format local 2 consistency maintained through response 3 different consumers may want a different timezone/format/locale
This is a sample query I think you're describing that is resolved on the server side:
query GetUser(userId: ID!) {
SignUpDate(format: String) : String
EnrollmentDate(format: String) : String
CancellationDate(format: String) : String
}
Right off the bat this is solved because it is the consumer that is writing the model -- so they have all control over the definitions.
As represented by models using arguments:
class User {
@GraphQLArgument(name="format", value="yyyy-mm-dd")
public String SignUpDate;
@GraphQLArgument(name="format", value="yyyy-mm-dd")
public String EnrollmentDate;
@GraphQLArgument(name="format", value="yyyy-mm-dd")
public String CancellationDate;
}
@GraphQLOperation(name="getSubscribers")
class GetHistoricalSubscribers {
public User[] Subscribers;
}
Above meets requirements 1 and 2 because the consumer defines the model and can specify their default values to match their needs. Requirement 3 is met by the very nature that each consumer is defining their own models: thus 1 & 2 again.
We can also accommodate exceptions to the desired default format, given the same model definition:
@GraphQLOperation(name="getSubscribers", variables={@GraphQLVariable(name = "format", scalar="String")})
class GetHistoricalSubscribers {
public User[] Subscribers;
}
// when building the request we can then bind a dateFormat variable to the arguments (though variable isn't needed, it could just be set arguments)
builder.setVariables(new Variable("dateFormat", "mm/dd/yy"));
builder.setArguments("GetHistoricalSubscribers.Subscribers.SignUpDate", new Argument("format", null, "dateFormat"));
builder.setArguments("GetHistoricalSubscribers.Subscribers.EnrollmentDate", new Argument("format", null, "dateFormat"));
Again this would be for exceptional occasions -- because why wouldn't the consumer define the model w/ the desired format in the first place?
So is your goal for declaring variables at an operation level over class level to keep from having variations like userId/UserID/UserId all in the same query like shown here myQuery($userId: String, $UserID: String, $UserId: String)
?
My primary reasons for defining the variables at any level are to keep closely synced with the GraphQL specification. Defining variables at the top level also means shifting the weight from setting them in the annotation to defining them through the dot notation path, which slightly concerns me. Since the dot notation seems a lot more likely to lead to a bad path or a typo. I'm pretty torn on this part actually, the weight is pretty heavy on both pros/cons here
So is your goal for declaring variables at an operation level over class level to keep from having variations like userId/UserID/UserId all in the same query like shown here myQuery($userId: String, $UserID: String, $UserId: String)?
It's one concern. Another concern is that arguments can have default values while variables do not. A model with a bunch of properties defined w/ sane defaults is easier to use than a model with a bunch of variables.
With the following model definition:
class User {
@GraphQLArgument(name="format", value="yyyy-mm-dd")
public String SignUpDate;
@GraphQLVariable(name="format", scalar="String")
public String EnrollmentDate;
}
Assuming I use request builder to make a query from the model and don't set any arguments or variables.
GraphQLRequestEntity requestEntity = GraphQLRequestEntity.Builder()
.headers(headers)
.url(this.endpointUrl);
.request(User.class)
.build()
What format will SignUpDate be? Look at the model: defaults to "yyyy-mm-dd". What format will EnrollmentDate be? Dunno, what ever the default value is in the spec. Go look at the spec. Because the model was defined with a variable more work is required in the request builder to specify good defaults.
My primary reasons for defining the variables at any level are to keep closely synced with the GraphQL specification.
To me -- that's what arguments do and more accurately. The model defines what arguments are available, as well as sane defaults. GraphQL schemas define arguments on fields (not variables). Consumers define variables with operations to be used as argument values.
There's also a weird constraint imposed on variables if you use them at the property level. For graphQL, variables are arbitrarily named placeholders that the consumer gets to define when they craft their operation. Ie query User($myUserIdVariable:ID!) { User(UserID: $myUserIdVariable){
. As is, because request builder translates property variable definitions to argument names -- defining a variable at the property level, the consumer is constrained to using the name that matches the argument for the property. This is a very subtle constraint but its just another thing that can trip a consumer up when writing their models.
Defining variables at the top level also means shifting the weight from setting them in the annotation to defining them through the dot notation path, which slightly concerns me. Since the dot notation seems a lot more likely to lead to a bad path or a typo.
Yeah, this is true and using dot path to set arguments and bind variables does feel clunky.
I'm pretty torn on this part actually, the weight is pretty heavy on both pros/cons here
So, I think I've got a compromise. This is already supported in the PR unintentionally -- but:
@GraphQLVariable(s)
from property annotations@GraphQLArgument
has the attribute variable
of type @GraphQLVariable
@GraphQLOperation
has optional variables attribute and picks up variables defined in @GraphQLArgument
class MyUserQuery {
public User user;
}
class User {
// if you want to define variables @ property level
@GraphQLArgument(name="format", variable=@GraphQLVariable(name = "dateFormat", Scalar = "String"))
public String SignUpDate;
// if you want to define them at the operation
@GraphQLArgument(name="format", value="yyyy-mm-dd")
public String EnrollmentDate;
}
Because we've defined the variable with the argument we can decide if we should render the statement with variable substitution based on if the builder has variables defined.
builder.request(MyUserQuery.class).build();
Output
query MyUserQuery { User { SignUpDate(format: "yyyy-mm-dd") EnrollmentDate(format: "yyyy-mm-dd") } }
When variables have been set in the builder, we render the substitution in the query.
builder.request(MyUserQuery.class)
.setVariables(new Variable("dateFormat", "mm/dd/yy")
.build();
Output
query MyUserQuery($dateFormat: String) { User { SignUpDate(format: $dateFormat) EnrollmentDate(format: "yyyy-mm-dd") } } variables { "$dateFormat": "mm/dd/yy" }
Properties with arguments defined not including a variable definition can be bound at request time.
builder.request(MyUserQuery.class)
.setVariables(new Variable("dateFormat", "mm/dd/yy")
.setArguments("MyUserQuery", new Argument("EnrollmentDate", null, "dateFormat"))
.build();
Output
query MyUserQuery($dateFormat: String) { User { SignUpDate(format: $dateFormat) EnrollmentDate(format: $dateFormat) } } variables { "$dateFormat": "mm/dd/yy" }
If we remove the ability to set variables at any level then we break away from the GraphQL specification, I see the concerns with it being more advanced and difficult to use than arguments but they are two distinct features not in my control and the discussion may be better moved there. If the goal is to have default values defined in the models, Arguments may be the best case, but I can also get behind an operation annotation setting defaults on variables since that should be restricted to the top level of the query - where variables are passed in.
I'll be opening a couple PRs tonight:
variables
in @GraphQLProperty
@GraphQLArgument(s)
/@GraphQLVariable(s)
available at a class levelHopefully that adds any of the missing features you mentioned and then we can look at the PR you raised and use the Operation annotation for setting default values at the top level
Unlike GraphQLProperty, GraphQLVariable(s) isn't class level -- I'm not sure how I'm suppose to use them to achieve my query/mutation.
I'm trying to build the following mutation
Or A more general usage example for Queries:
Expected Uses
I was anticipating a model definition like (but illegal to put variables @ class level):
Available Uses
These are my (legal) options but all provide the wrong result.
Test Case
Input variable model
Result Model
1 Property and Values set inside the class on the result model property
MutationCreate2FA
Output:
2 Property on the class but variables on a result model property
MutationCreate2FA
Output:
3 Variables only specified when building request
MutationCreate2FA
Output (close, but the builder doesn't inject the variables):
Preferred usage
If I step back from the docs a little and imagine how I might specify query's and mutations with variables I come up with:
Output
I think this preferred usage can be provided and be backwards compatible w/ current usage.
I've introduced an annotation
@GraphQLOperation
which is optional (if you don't need variables) in your query/mutation. This would remove the need for@GraphQLVariables
(altogether) and@GraphQLVariable
would only be used in top level operation annotations (not on models or on model properties).I've also added an optional
variable
parameter to the@GraphQLArgument
which associates the argument to a specified variable. The addition allows properties to be defined as arguments that can be bound to variables later which reduces unwanted coupling from the model to the operation.