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

GraphQlQueryParameter<T> and QueryBuilderParameter<T> design questions #131

Open
gao-artur opened this issue Feb 2, 2023 · 2 comments
Open

Comments

@gao-artur
Copy link
Contributor

gao-artur commented Feb 2, 2023

Hi. I have a few questions regarding GraphQlQueryParameter<T> and QueryBuilderParameter<T> design.
The first one is used to create the parametrized query (WithParameter method). The second is to provide an inline arguments value or parameter name (With{FIELD} methods).

  1. GraphQlQueryParameter<T> has a few constructors. When using
public GraphQlQueryParameter(string name, string graphQlTypeName = null)

I expect the nullability to be inferred from the T nullability, including respecting the NRT if enabled. But instead, it always creates nullable arguments. For example, Int instead of Int!. It also seems to have a bug in the following line because ? shouldn't be appended to the graphql type name:

var nullableSuffix = nullableUnderlyingType == null ? null : "?";

So, my question is if there is any reason why this constructor won't be able to infer the nullability from the T?

  1. There is an additional constructor.
public GraphQlQueryParameter(string name, T defaultValue, bool isNullable = true)

That allows to define the nullability manually, but T defaultValue is required in this constructor. Any reason why I can't override the nullability without specifying the default value?

  1. The QueryBuilderParameter<T> doesn't have public constructors. This forces me to use GraphQlQueryParameter<T> and provide redundant parameters like graphql type name or isNullable and enforces validations that are not relevant here. I understand that I can reuse an instance that was passed into WithParameter but it makes the API "less fluent". I also understand that adding a public constructor with a single argument for a parameter name will create ambiguity with a private constructor when T is string, but maybe design can be changed slightly to allow simple creation of the QueryBuilderParameter<T> instance.
@Husqvik
Copy link
Owner

Husqvik commented Feb 4, 2023

one problem with the nullability is that it's not only specified by the type itself. You can pass the value hard coded in the query string as within the parameter object so null value can be passed to parameter that is defined as not null.

public GraphQlQueryParameter(string name, string graphQlTypeName, T defaultValue) probably should be market obsolete, it comes from the time where there wasn't complete bidirectional mapping between the GQL types and the generated .NET types. Hopefully that is now complete so it shouldn't be any reason to pass graphQlTypeName manually.

@gao-artur
Copy link
Contributor Author

Yeah, I understand that user will always find a way to misuse the library :)
But I think, providing defaults that will hit most of the time is better than just make everything nullable by default.
And workarouds for possible bugs is a good enough reason to keep the graphqlTypeName
#130

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