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

Possibly use raw SQL to convert to jsonb (maybe not always) #149

Closed
dmitriid opened this issue Jun 2, 2023 · 9 comments
Closed

Possibly use raw SQL to convert to jsonb (maybe not always) #149

dmitriid opened this issue Jun 2, 2023 · 9 comments
Labels
bug Something isn't working

Comments

@dmitriid
Copy link

dmitriid commented Jun 2, 2023

Describe the bug

When converting a text column to jsonb,

ERROR 42804 (datatype_mismatch) column "<column name>" cannot be cast automatically to type jsonb

hint: You might need to specify "USING <column name>::jsonb".

The migration for the column is

defmodule Telepai.Repo.Migrations.UpdateSessionSettingField do
  use Ecto.Migration

  def up do
    alter table(:sessions) do
      modify :settings, :map
    end
  end

  def down do
    alter table(:sessions) do
      modify :settings, :text
    end
  end
end

I honestly can't tell you if this is always the case, but it looks like at least for text columns the conversion should include USING <column name>::JSONB

ALTER TABLE sessions ALTER COLUMN settings TYPE JSONB USING settings::JSONB

If I'm not mistaken, the only way to do that with migrations is to use a raw execute

  def up do
    execute "..."
  end

To Reproduce

  • Create migrations for the resource before and after the change:
defmodule Telepai.AI.Session do
  use Ash.Resource,
    data_layer: AshPostgres.DataLayer

  postgres do
    table("sessions")
    repo(Telepai.Repo)
  end

  attributes do
    uuid_primary_key(:id)

-   attribute :settings, :string do
+   attribute :settings, :map do 
      allow_nil?(false)
    end

    create_timestamp :inserted_at
  end
end
  • Run migrations

Expected behavior

Migration should run with no issue

Runtime

  • Elixir version: Elixir 1.14.4
  • Erlang version: Erlang/OTP 25 [erts-13.2] [source] [64-bit]
  • OS: MacOS Ventura 13.4 (22F66)
  • Ash version: {:ash, "~> 2.9.6"}
  • any related extension versions: {:ash_postgres, "~> 1.3.25"}
@dmitriid dmitriid added bug Something isn't working needs review labels Jun 2, 2023
@zachdaniel
Copy link
Contributor

Its an interesting idea, we could potentially do something like this by default. I think its reasonable that some migrations will have to have manual intervention, but this seems like something we could potentially detect and provide a better default for.

@zachdaniel
Copy link
Contributor

In this case, though, it would produce a jsonb string and not a map though.

@dmitriid
Copy link
Author

dmitriid commented Jun 2, 2023

The migrations generator produced modify :settings, :jsonb for me when I changed attribute type from string to map🤔

@zachdaniel
Copy link
Contributor

Yeah, but a string is a valid jsonb. So my point is even with USING column:jsonb it will still not be an actual object under the hood.

@dmitriid
Copy link
Author

dmitriid commented Jun 2, 2023

Ah! I see what you mean.

I missed this because in my case I was storing Jason.encoded strings anyway :) But if there were other strings, it would either fail, or would fail on read 🤔

No idea how to solve this properly besides generating warnings or comments during migration or when generating migrations.

@zachdaniel
Copy link
Contributor

Yeah, I mean honestly thats what happens when you run the migrations, the data store tells you that you can't actually do that transition and have to figure out some other way to do what you want. I'm not sure if the migration generator should try to figure this one out since it can't know what the right answer is.

@zachdaniel
Copy link
Contributor

I think we can't reliably do this kind of thing currently. Folks will need to edit the migrations themselves.

@dmitriid
Copy link
Author

awww shucks :(

Well, it will probably affect a very small number of migrations anyway, and the solution is quite easy.

@zachdaniel
Copy link
Contributor

Yeah, exactly. Its possible that in the future we could do it with something interactive in cases we can detect an issue, but the juice is unlikely to be worth the squeeze.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants