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

Correctly convert null defaults #117

Merged
merged 13 commits into from
Jun 10, 2022
Merged

Correctly convert null defaults #117

merged 13 commits into from
Jun 10, 2022

Conversation

aidenfoxx
Copy link
Contributor

Oracle and SQLite can return unquoted string null to indicate a default null value. Currently there is an issue in Directus if you change a columns default to NULL it will always return as string null which is not the correct behavior.

lib/dialects/sqlite.ts Outdated Show resolved Hide resolved
@aidenfoxx
Copy link
Contributor Author

@Oreilles If you have a second I'd be interested to hear your thoughts on the PR?

The idea behind the PR is to update "data_default" to consistently return null instead of string null to avoid flaky workarounds in Directus. I also removed casting for "data_default" to keep the schemas consistent. The logic being if the user wants to cast the value they can do so based on the "data_type".

Copy link
Contributor

@Oreilles Oreilles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly from the Directus standpoint, it is a good idea to tackle the default value parsing duplication which happens too in https://github.com/directus/directus/blob/main/api/src/utils/get-default-value.ts, including duplicate check for "NULL" strings and stripping of wrapping quotes . But it means this is a slightly significant change for this library.. If it only was an issue with Oracle, and specifically null values, only patching that small quirk could also do the job, while keeping the parsing feature in the schema inspector.. It ultimately depends on what the end goal is. 🙂

lib/dialects/cockroachdb.ts Outdated Show resolved Hide resolved
lib/dialects/cockroachdb.ts Show resolved Hide resolved
lib/dialects/mssql.ts Outdated Show resolved Hide resolved
lib/utils/strip-quotes.ts Show resolved Hide resolved
lib/dialects/postgres.ts Outdated Show resolved Hide resolved
@aidenfoxx
Copy link
Contributor Author

aidenfoxx commented Jun 9, 2022

So the end goal would be to not have to rely on workarounds in Directus to handle null column defaults, since that feels like treating the symptoms rather than the cause.

Removing casting on the column defaults does feel like the most controversial change. But I think it's better to have a consistent response rather than having casting which is inconsistently applied to different schemas.

lib/dialects/mssql.ts Outdated Show resolved Hide resolved
@aidenfoxx aidenfoxx changed the title Fixed long standing issue with null defaults in Oracle and SQLite Correctly convert null defaults Jun 9, 2022
@rijkvanzanten rijkvanzanten merged commit 184c311 into knex:master Jun 10, 2022
joselcvarela pushed a commit to joselcvarela/knex-schema-inspector that referenced this pull request May 24, 2023
* Fixed long standing issue with null defaults in Oracle and SQLite

* Minor tweaks

* Made cockroach happy

* Fixed bad autocomplete

* Handle MSSQL defaults null

* Removed unused file

* Made null handling more consistent

* Update lib/dialects/sqlite.ts

* Stop parsing JSON defaults

* Fixed PR comments

* Update lib/dialects/mssql.ts
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

Successfully merging this pull request may close these issues.

None yet

3 participants