erlangbureau / jamdb_oracle

Oracle Database driver for Erlang
MIT License
106 stars 48 forks source link

Quoting Names - New Option #97

Closed mlh758 closed 3 years ago

mlh758 commented 3 years ago

I've been migrating a Rails app to Elixir and it uses Oracle. I noticed that some column names no longer work like date and comment. The issue is that the Oracle Rails adapter automatically upcases and quotes names while this adapter passes them through as given.

Would a PR that changes this behavior be acceptable? Oracle is normally case insensitive unless you use quotes so I think it would be passive as currently the adapter forbids anything containing quotes. My proposal is essentially to change quote_name in jamdb_oracle_query.ex to:

defp quote_name(name) do
  if String.contains?(name, "\"") do
    error!(nil, "bad field name #{inspect name}")
  end
  if Application.get_env(:jamdb_oracle, :upquote_columns, false) do # can probably find a better name for option
    ["\"#{String.upcase(name)}\""]
  else
    [name]
  end
end

Edit - included it with the configuration option.

vstavskyi commented 3 years ago

Try without name checking commit query = from u in "\"Users\"", select: u.name

Just in case, please check returning results of INSERT, UPDATE, DELETE, MERGE statements and BEGIN .. END blocks. commit

mlh758 commented 3 years ago

Pulled down master with those two commits:

from(m in "\"METRICS\"", select: m.id) |> Repo.all seems to work fine.

I tried to test with insert but still get my original error about an invalid column name.

My initial issue is that we have a column named COMMENT and unless the name is quoted Oracle will reject an insert with ORA-01747: invalid user.table.column, table.column, or column specification. My proposed change to quote_name fixes the issue for me and I can insert/update/select without issues.

mlh758 commented 3 years ago

I get that this is your library though, so I'm hoping the option will make it passive enough to bring the feature in.

vstavskyi commented 3 years ago

I got it.

CREATE TABLE newTable1 
    ( 
        col1 INTEGER 
    );

CREATE TABLE "newTable2"
    ( 
        "col2" INTEGER 
    );

SELECT col1 from newTable1 where col1 in (SELECT "col2" from "newTable2")

With your new option ALL object names will be in uppercase, and you'll get ORA error.

SELECT "COL1" from "NEWTABLE1" where "COL1" in (SELECT "COL2" from "NEWTABLE2")

Maybe upcase must be called in application but not in driver.

mlh758 commented 3 years ago

The only options I see in Ecto to change it application side are source and @field_source_mapper both of which need to return an atom so I can't specify quoting the column name there.

I could make the proposed change slightly fancier and just copy what the Oracle adapter for Rails is doing. They upcase and quote the column name unless it already contains quotes in which case they delete the quotes and wrap it in quotes anyway. That should handle someone with a column name of "col2".

vstavskyi commented 3 years ago

Now adapter passes names through as given. But in Ecto name checking remained the same.

Post example of migration code which throws the error.

mlh758 commented 3 years ago

I'm working with an existing database, it's not a migration that gives me problems but running queries/inserts/updates against an existing schema.

Something like:

CREATE TABLE "ACTIONS" 
(   "ID" NUMBER(38,0), 
"USER_ID" NUMBER(38,0), 
"ACTION" VARCHAR2(255 CHAR), 
"COMMENT" VARCHAR2(1000 CHAR), 
"CREATED_AT" TIMESTAMP (6), 
"UPDATED_AT" TIMESTAMP (6), 
"METRIC_ID" NUMBER(38,0)
);

And then use Ecto to try and insert into it. I need the adapter to wrap the column names in quotes or it will fail because COMMENT is a reserved word. My initial proposal puts this behind a configuration option (disabled by default) to avoid changing existing behavior but anyone who has to work with a schema that uses reserved words somewhere would need that quoting functionality.

vstavskyi commented 3 years ago

link

The most recent version has probably a nicer way to deal with this - leveraging the source: option to field/3. You can declare field :foo, :string, source: :FOO_BAR. This allows using :foo in code and the database will receive FOO_BAR.

defmodule Jamdb.Actions do
  use Ecto.Schema

  @primary_key {:id, :integer, autogenerate: false}
  schema "\"ACTIONS\"" do
    field :comment, :string, source: :'"COMMENT"'
  end

end

Jamdb.Actions |> Jamdb.Repo.all SELECT t0.id, t0."COMMENT" FROM "ACTIONS" t0 []

from(m in Jamdb.Actions, select: m.comment) |> Jamdb.Repo.all SELECT t0."COMMENT" FROM "ACTIONS" t0 []

from(m in "\"ACTIONS\"", select: m.'"COMMENT"') |> Repo.all

Almost!?

mlh758 commented 3 years ago

Ah I see, I didn't realize you could do that with atoms. I'll check this out tomorrow.

mlh758 commented 3 years ago

This works! Thank you.