mickhansen / graphql-sequelize

GraphQL & Relay for MySQL & Postgres via Sequelize
MIT License
1.9k stars 172 forks source link

Question about fields with default value #649

Closed Yehonal closed 5 years ago

Yehonal commented 5 years ago

Hi

I've a mysql database with hundreds of tables and i'm trying to bulk create graphql schemas and resolvers automatically. I'm trying to configure graphql-sequelize to help me with this job but i've found an issue:

when graphql-sequelize converts the Sequelize model in graphql schema it doesn't take care about fields with non-null fields with default values and wrap them with GraphQLNonNull, Shouldn't be it avoided or configurable?

Is it possible to avoid GraphQLNonNull when default value exist for that field?

mickhansen commented 5 years ago

Why would you not want GraphQLNonNull in that case? In an case, no it's not possible, but you can skip the attribute and define it manually.

Yehonal commented 5 years ago

As i said i've hundreds of tables with thousands of attributes defined as "non-null" with default value, so i cannot do it manually. Using non-null with default value means that you can avoid to specify field value in your insert query but you cannot assign a NULL. It's a normal behaviour for databases and would be nice that graphql-sequelize can be configured to allow null values in such case, so the responsibility of data consistency is assigned to the database.

I'm currently using allowNull for now but it includes also the case where there is no default value in sequelize/mysql.

mickhansen commented 5 years ago

If the database has a non null constraint i don't see a generic reason to ever not have the graphql field be non null as well, the validation would never fail.

You could simply copy the attribute fields method from this library and make your own if it's a productivity issue for you.

Yehonal commented 5 years ago

I'm using graphql-sequelize to generate input types so if you wrap that field with GraphQLNonNull, the database/sequelize default value is really useless because you cannot omit that field in graphql mutation query.

You should implement the default value for graphql instead of GraphQLNonNull wrapping (here is an example: https://graphql.org/graphql-js/type/#example-5)

Or am I missing something?

mickhansen commented 5 years ago

I see, you're the first one i've heard about that uses it for input types, i assumed we were talking about field types as that's what it's commonly used for.

I'd suggest writing your own input fields generator.

Yehonal commented 5 years ago

I see, you're the first one i've heard about that uses it for input types, i assumed we were talking about field types as that's what it's commonly used for.

I'd suggest writing your own input fields generator.

ok thank you. I've copied your attributeFields file using this conditon:

            if (typeof attribute.defaultValue == "undefined" && (attribute.allowNull === false || attribute.primaryKey === true)) {
                memo[key].type = new graphql_1.GraphQLNonNull(memo[key].type);
            }

it's very useful for input types but maybe for schema type is not suggested. I'm not very skilled with GraphQL so i would ask you: in your opinion, what kind of implications are there if you have a schema without non-null graphql wrapper in front of a non-null database field with default value? (I don't know if I made this clear)

Yehonal commented 5 years ago

I've created a pull request with option to enable such feature, if anyone would use it for input type generation:

https://github.com/mickhansen/graphql-sequelize/pull/650

for example i've modified graphql-sequelize-crud utility to use it and now i'm able to create even crud methods automatically: https://github.com/Yehonal/graphql-sequelize-crud

thanks for support

mickhansen commented 5 years ago

Well i agree that it makes little sense to have a NonNull input field infront of a field with a default value. However in general i haven't recommended that anyone use auto generated schemas in a long while as it to me goes against the benefits and nature of graphql

wallzero commented 5 years ago

I am interested in using this myself.