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

fix: keep fields of custom_index in format that they were provided #195

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

vonagam
Copy link
Contributor

@vonagam vonagam commented Jan 22, 2024

Right now fields of custom_index are first transformed into atoms and then during migration generation into strings.

I used a field named "order" and encountered an error because of this. Ecto generates different statements depending on whenever a field was provided as a string or as an atom:

create index(:some_table, ["order"]) # invalid
create index(:some_table, [:order]) # valid

For atoms the resulting name will be in quotes, for strings they will be left as is (and it, for example, allows specifying things like "name gin_trgm_ops" for full-text search usage).

So it makes sense to keep user provided input as is and not convert it into atoms or strings.

Also made RemoveCustomIndex operation to use reverse direction of AddCustomIndex to avoid repetition.

@vonagam vonagam force-pushed the fix-custom-index-fields branch from 6eed9e5 to e9a4a92 Compare January 22, 2024 14:39
@zachdaniel
Copy link
Contributor

I think you're going to have to handle this when loading snapshots and encoding snapshots as well. Once we load the json snapshot, they will always end up being strings.

@vonagam
Copy link
Contributor Author

vonagam commented Jan 22, 2024

Hm... makes sense. Can close then.

Alternative though maybe to always use atom form except when there is a white space involved.

@zachdaniel
Copy link
Contributor

Oh, I think it's solvable, we just need to do a bit of extra work. When encoding we should store it as {"type": "atom" | "string", value: "value"}, and then decode it appropriately.

@zachdaniel
Copy link
Contributor

And handle if its just a string for backwards compatibility.

@vonagam vonagam force-pushed the fix-custom-index-fields branch from e9a4a92 to e851f4b Compare January 22, 2024 15:50
@vonagam
Copy link
Contributor Author

vonagam commented Jan 22, 2024

Ok, implemented such thing.

@zachdaniel
Copy link
Contributor

awesome work! 🚀 Thank you for your contribution! 🚀

@zachdaniel zachdaniel merged commit 8f3100a into ash-project:main Jan 22, 2024
52 checks passed
@vonagam vonagam deleted the fix-custom-index-fields branch January 22, 2024 16:33
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.

2 participants