Insert - Ecto.Adapters.SQL.Connection

[Danwhy]

Part of #38

https://hexdocs.pm/ecto_sql/Ecto.Adapters.SQL.Connection.html#c:insert/6

For this function, we need to generate a cid based on the given data, as well as create an "entry id" for ease of lookup.

[RobStallion]

Comment schema in app

  schema "comments" do
    field :comment, :string
    field :comment_id_no, :string
    field :show, :boolean

    timestamps()
  end

Calling Repo.insert in app (purposefully leaving nil empty)

Repo.insert(%UsingAlogAdapter.Comments{comment: "hi", comment_id_no: "1"})

Newly defined insert/6 in AlogAdapter module

  def insert(adapter_meta, %{source: source, prefix: prefix}, params, on_conflict, returning, opts) do
    params = params ++ [show: true]  # <---- Adding :show as :true in the adapter

    {kind, conflict_params, _} = on_conflict
    {fields, values} = :lists.unzip(params)
    sql = @conn.insert(prefix, source, fields, [fields], on_conflict, returning)
    Ecto.Adapters.SQL.struct(adapter_meta, @conn, sql, :insert, source, [], values ++ conflict_params, kind, returning, opts)
  end

Logs from the terminal

INSERT INTO "comments" ("comment","comment_id_no","inserted_at","updated_at","show") VALUES ($1,$2,$3,$4,$5) RETURNING "id" ["hi", "1", ~N[2019-02-19 20:01:40], ~N[2019-02-19 20:01:40], true]

Changeset returned from Repo.insert

{:ok,
 %UsingAlogAdapter.Comments{
   __meta__: #Ecto.Schema.Metadata<:loaded, "comments">,
   comment: "hi",
   comment_id_no: "1",
   id: 51, # <----- ID no. entered into the db
   inserted_at: ~N[2019-02-19 20:01:40],
   show: nil,
   updated_at: ~N[2019-02-19 20:01:40]
 }}

# notice that the changeset says show is nil. I think this is actually the behaviour we
# will want in the adapter. If we are going to use the adapter to manually add the field
# entry_id (what I am doing with show here), then it will not be part of the users schema,
# meaning that they do not need to see it in their changeset

Log of Repo.get(Comments, 51)

iex()> Repo.get(Comments, 51)
[debug] QUERY OK source="comments" db=3.8ms queue=1.4ms
SELECT DISTINCT ON (c0."comment_id_no") c0."id", c0."comment", c0."comment_id_no", c0."show", c0."inserted_at", c0."updated_at" FROM "comments" AS c0 WHERE (c0."id" = $1) [51]
%UsingAlogAdapter.Comments{
  __meta__: #Ecto.Schema.Metadata<:loaded, "comments">,
  comment: "hi",
  comment_id_no: "1",
  id: 51,
  inserted_at: ~N[2019-02-19 20:01:40],
  show: true,  # <--------- Now showing true
  updated_at: ~N[2019-02-19 20:01:40]
}

We can see that when we select the new entry from the db is has show as true.

The shows that we can add/manipulate the values before they get entered into the db using an adapter. 🎉😄

[RobStallion]

For more info on how I got to this point see RobStallion/alog_adapter#2

[RobStallion]

Next steps

• Add insert/6 to the Alog module.
• Replace params = params ++ [show: true] with a function that adds the cid and entry_id values to the list of params.
  • add CID into project deps so we can create CID from params
  • Create a map from the params, excluding inserted_at & updated_at, to be used to create the CID.
  • Take the first 3 characters of the CID and use as the entry_id.
  • Make sure the CID is unique (needs to be unique for insert function)
  • Make sure the entry_id is unique (needs to be unique for insert function)
  • If entry_id is not unique, and another character and check for uniqueness again (repeat this step as many times as needed until we get a unique entry id)

Make sure the entry_id is unique

I think that this will be the most tricky part as it means that we will somehow need to query the database. I think that this will be tricky as we will need to be able to create a query struct that will allow us to query the db from the adapter (something that we can normally do in an app with Repo.all(User) for example will not work as it creates the struct incorrect. I think it will be a similar issue as the one we encountered with all and subquery)

[Danwhy]

We should be able to make sure the CID is unique using a unique index. In the execute_ddl functions when we create a table we define the cid column, we can also add a unique index for it.

All we need to do then is make sure the entry id is unique, which might require some querying

[Danwhy]

@RobStallion Another thing to note is that we need to make sure the timestamp fields are present in the params when we insert.

I automatically create the columns during the migration, but they will just be null if timestamps is not in the schema. We just need to check the params, and insert them if the user hasn't.

[RobStallion]

@Danwhy Thanks for responding on this. I realised that I made a typo sorry 😞

I was meant to say "Make sure the entry_id" is unique". I will update my earlier comment.
You mentioned what I thought was the issue (regardless of me being an egg 🥚)

All we need to do then is make sure the entry id is unique, which might require some querying

@Danwhy @SimonLab Do you have any thoughts on how we can do the querying to check if the entry_id is unique?

[RobStallion]

@Danwhy Regarding this comment, good thinking 🧠

I'll make sure I add a check for that. Should be a fairly simple step 👍

[Danwhy]

The query should be quite simple, so we might be able to just create a sql string: SELECT entry_id where entry_id = 'f' for example, then pass it to the execute function.

We can do this recursively until we find a non existing entry id: SELECT entry_id where entry_id = 'fy',
SELECT entry_id where entry_id = 'fyZ' etc.

[RobStallion]

@Danwhy Can we write a query like that in the adapter?

I thought that it would give us the error we were getting when trying to use subquery. I will test this now and get back to you

[Danwhy]

The error we were getting in all is because we were trying to manipulate an existing ecto query using lots of existing functions.

For this we just need a simple sql command with only one parameter, so there's no reason to use an ecto query.

[RobStallion]

Did the same Repo.insert call I have for the above tests.

Came through to the insert function I added to the adapter.

  import Ecto.Query, only: [from: 1]
  def insert(adapter_meta, %{source: source, prefix: prefix}, params, on_conflict, returning, opts) do
    params = params ++ [show: true]

    IO.inspect(source, label: "source ===>") # logs: source ===>: "comments"

    AlogAdapter.Connection.all(from c in source, select: c.id) # Causing the error below
    |> IO.inspect(label: "-----> Result of repo all")

    {kind, conflict_params, _} = on_conflict
    {fields, values} = :lists.unzip(params)
    sql = @conn.insert(prefix, source, fields, [fields], on_conflict, returning)
    Ecto.Adapters.SQL.struct(adapter_meta, @conn, sql, :insert, source, [], values ++ conflict_params, kind, returning, opts)
  end

Error message

** (exit) an exception was raised:
    ** (ArgumentError) argument error
        :erlang.tuple_size(nil)
        (ecto_sql) lib/ecto/adapters/postgres/connection.ex:655: Ecto.Adapters.Postgres.Connection.create_names/1
        (ecto_sql) lib/ecto/adapters/postgres/connection.ex:107: Ecto.Adapters.Postgres.Connection.all/1
        (alog_adapter) lib/alog_adapter.ex:55: AlogAdapter.insert/6
        (ecto) lib/ecto/repo/schema.ex:651: Ecto.Repo.Schema.apply/4
        (ecto) lib/ecto/repo/schema.ex:264: anonymous fn/15 in Ecto.Repo.Schema.do_insert/3
        (using_alog_adapter) lib/using_alog_adapter_web/controllers/page_controller.ex:15: UsingAlogAdapterWeb.PageController.index/2
        (using_alog_adapter) lib/using_alog_adapter_web/controllers/page_controller.ex:1: UsingAlogAdapterWeb.PageController.action/2
        (using_alog_adapter) lib/using_alog_adapter_web/controllers/page_controller.ex:1: UsingAlogAdapterWeb.PageController.phoenix_controller_pipeline/2
        (using_alog_adapter) lib/using_alog_adapter_web/endpoint.ex:1: UsingAlogAdapterWeb.Endpoint.instrument/4
        (phoenix) lib/phoenix/router.ex:275: Phoenix.Router.__call__/1
        (using_alog_adapter) lib/using_alog_adapter_web/endpoint.ex:1: UsingAlogAdapterWeb.Endpoint.plug_builder_call/2
        (using_alog_adapter) lib/plug/debugger.ex:122: UsingAlogAdapterWeb.Endpoint."call (overridable 3)"/2
        (using_alog_adapter) lib/using_alog_adapter_web/endpoint.ex:1: UsingAlogAdapterWeb.Endpoint.call/2
        (phoenix) lib/phoenix/endpoint/cowboy2_handler.ex:34: Phoenix.Endpoint.Cowboy2Handler.init/2
        (cowboy) /Users/robertfrancis/Code/spike/using_alog_adapter/deps/cowboy/src/cowboy_handler.erl:41: :cowboy_handler.execute/2
        (cowboy) /Users/robertfrancis/Code/spike/using_alog_adapter/deps/cowboy/src/cowboy_stream_h.erl:296: :cowboy_stream_h.execute/3
        (cowboy) /Users/robertfrancis/Code/spike/using_alog_adapter/deps/cowboy/src/cowboy_stream_h.erl:274: :cowboy_stream_h.request_process/3
        (stdlib) proc_lib.erl:249: :proc_lib.init_p_do_apply/3

I figured that this would be the most simple query I could write to test the theory.

code in app

    Repo.all(from c in "comments", select: c.id)

This query in my application works fine if the adapter is left like so...

  def insert(adapter_meta, %{source: source, prefix: prefix}, params, on_conflict, returning, opts) do
    params = params ++ [show: true]
    {kind, conflict_params, _} = on_conflict
    {fields, values} = :lists.unzip(params)
    sql = @conn.insert(prefix, source, fields, [fields], on_conflict, returning)
    Ecto.Adapters.SQL.struct(adapter_meta, @conn, sql, :insert, source, [], values ++ conflict_params, kind, returning, opts)
  end

If anyone sees anything obviously wrong with this then please comment.

[Danwhy]

AlogAdapter.Connection.all will just return a sql string that we'll then have to execute. There's no need for us to call it as the string is so simple. Try just executing a sql string:

Ecto.Adapters.SQL.query(adapter_meta, "SELECT id FROM #{source}", [])

[RobStallion]

@Danwhy Worked like a charm thanks 👍

[RobStallion]

  def insert(adapter_meta, %{source: source, prefix: prefix}, params, on_conflict, returning, opts) do
    params_map = Enum.into(params, %{})
    map_for_cid = Map.drop(params_map, [:inserted_at, :updated_at])

    cid = Cid.cid(map_for_cid)
    entry_id = create_entry_id(source, adapter_meta, cid, 2)

    params =
      params_map
      |> Map.put(:cid, cid)
      |> Map.put(:entry_id, entry_id)
      |> Enum.into([])

    {kind, conflict_params, _} = on_conflict
    {fields, values} = :lists.unzip(params)
    sql = @conn.insert(prefix, source, fields, [fields], on_conflict, returning)
    Ecto.Adapters.SQL.struct(adapter_meta, @conn, sql, :insert, source, [], values ++ conflict_params, kind, returning, opts)
  end

  defp create_entry_id(source, adapter_meta, cid, n) do
    entry_id = String.slice(cid, 0..n)
    entry_id_query = "SELECT * FROM #{source} where entry_id='#{entry_id}'"
    {:ok, results} = Ecto.Adapters.SQL.query(adapter_meta, entry_id_query, [])

    if results.num_rows == 0 do
      entry_id
    else
      create_entry_id(source, adapter_meta, cid, n+1)
    end
  end

  defp add_timestamps(params) do
    params
    |> Enum.into(%{})
    |> Map.put_new(:inserted_at, NaiveDateTime.utc_now())
    |> Map.put_new(:updated_at, NaiveDateTime.utc_now())
  end

[RobStallion]

The above adds the cid and entry_id to the database. Makes sure that the entry_id is unique.

Only thing I am slightly unsure of is this line...
|> Map.put(:cid, cid)

as in reality this key will be the id (not cid).

[Danwhy]

@RobStallion When I try to insert something into the database I'm getting the following error:

** (DBConnection.EncodeError) Postgrex expected a binary of 16 bytes, got "zb2rhmxXxMwxhqGdQvqQw1dok73xqkdyLb1gJHx1uUWBzxvZ1". Please make sure the value you are passing matches the definition in your table or in your query or convert the value accordingly.

Did you come across this at all?
I've set the primary key of my schema to be the :cid, with a type of :binary_id and autogenerate: true.

[Danwhy]

⬆️I had the column type in the migration also set to :binary_id, which Postgrex translates to a Postgres type of uuid, which is a specifically 16 bit binary.

I had to change the type of the column :binary, which Postgrex turns into a Postgres type of bytea, which is its generic binary type.

[RobStallion]

Getting an error when I try to run mix ecto.migrate

➜  alog git:(insert) ✗ MIX_ENV=test mix ecto.create
Compiling 11 files (.ex)
Generated alog app
The database for Alog.Repo has been created
➜  alog git:(insert) ✗ MIX_ENV=test mix ecto.migrate
10:20:57.375 [error] Could not update schema migrations. This error usually happens due to the following:

  * The database does not exist
  * The "schema_migrations" table, which Ecto uses for managing
    migrations, was defined by another library

To fix the first issue, run "mix ecto.create".

To address the second, you can run "mix ecto.drop" followed by
"mix ecto.create". Alternatively you may configure Ecto to use
another table for managing migrations:

    config :alog, Alog.Repo,
      migration_source: "some_other_table_for_schema_migrations"

The full error report is shown below.

10:20:57.378 [error] Postgrex.Protocol (#PID<0.203.0>) disconnected: ** (DBConnection.ConnectionError) client #PID<0.219.0> exited

[RobStallion]

Something in the insert/6 function that is created on this branch is causing the error.

Not sure what it is at the moment but if I comment out the function and then run the migrate command, it works as expected.

[RobStallion]

Didn't notice this error message first time round...

** (MatchError) no match of right hand side value: {:error, %Postgrex.Error{connection_id: 39622, message: nil, postgres: %{code: :undefined_column, file: "parse_relation.c", line: "3293", message: "column \"entry_id\" does not exist", pg_code: "42703", position: "39", routine: "errorMissingColumn", severity: "ERROR", unknown: "ERROR"}, query: "SELECT * FROM schema_migrations where entry_id='zb2'"}}
    lib/alog.ex:64: Alog.create_entry_id/4
    lib/alog.ex:39: Alog.insert/6
    (ecto) lib/ecto/repo/schema.ex:651: Ecto.Repo.Schema.apply/4
    (ecto) lib/ecto/repo/schema.ex:264: anonymous fn/15 in Ecto.Repo.Schema.do_insert/3
    (ecto) lib/ecto/repo/schema.ex:166: Ecto.Repo.Schema.insert!/3
    (ecto_sql) lib/ecto/migrator.ex:493: Ecto.Migrator.verbose_schema_migration/3
    (ecto_sql) lib/ecto/migrator.ex:164: Ecto.Migrator.async_migrate_maybe_in_transaction/6
    (ecto_sql) lib/ecto/migrator.ex:438: anonymous fn/5 in Ecto.Migrator.do_migrate/4

This should shed some more light on what is going wrong with the insert function

[RobStallion]

This error appears to be caused by the schema migrations table. This appears to relate to this comment from @Danwhy.

Going to move on for now

[Danwhy]

@RobStallion you'll need to add a clause for your insert function that forwards to the original insert if the source is "schema_migrations"

[RobStallion]

Beginning to add tests to this branch.

Migration file

  def change do
    create table(:comments, primary_key: false) do
      # cid & entry_id need to be removed later as they should be handled in execute_ddl I believe
      # timestamps are needed in alog but may or may not be in the schema.
      add(:cid, :string, primary_key: true)
      add(:entry_id, :string)
      add(:comment, :string)
      add(:deleted, :boolean, default: false)

      timestamps()
    end

Schema

  # I'd imagine we'll change string as the type
  @primary_key {:cid, :string, autogenerate: false}
  schema "comments" do
    field(:entry_id, :string)
    field(:comment, :string)
    field(:deleted, :boolean, default: false)
  end

I can call Repo.insert with a struct (normal ecto behaviour). See logs below...

Test

Repo.insert(%Comment{comment: "hi"})
|> IO.inspect(label: "===> Result of insert")

===> Result of insert: {:ok,
 %Alog.TestApp.Comment{
   __meta__: #Ecto.Schema.Metadata<:loaded, "comments">,
   cid: nil,
   comment: "hi",
   deleted: false,
   entry_id: nil
 }}

Repo.all(Comment)
|> IO.inspect(label: "===> Result of Repo.all")

===> Result of Repo.all: [
  %Alog.TestApp.Comment{
    __meta__: #Ecto.Schema.Metadata<:loaded, "comments">,
    cid: "zb2rhh7yN9jPXNoU95A2JJhufSHSFN7X2q45Jmu72CXjF2QJ9",
    comment: "hi",
    deleted: false,
    entry_id: "zb2"
  }
]

You can see that the record was inserted into the db. However, the struct returned from insert and all do not match.

[RobStallion]

If we are going to add the fields cid, entry_id and deleted ourselves when the migrations are run then these fields with not be in the users schema. This means that the result of insert will be (using the above as an example)...

{:ok,
 %Alog.TestApp.Comment{
   __meta__: #Ecto.Schema.Metadata<:loaded, "comments">,
   comment: "hi"
 }}

Questions

@Danwhy @nelsonic @SimonLab

Is this how you have envisioned the adapter working?
What fields would you expect a user to put in their comment schema?

[nelsonic]

@RobStallion seems good.

Related but not urgent: I feel we may need to standardise on :id as the identifier rather than :entry_id simply on the basis that more people will expect the field to be called :id ... thoughts?

[RobStallion]

@nelsonic Is this a quote from another issue or is this your response to above? (the 'insert quote' > made me unsure)

I'm happy to rename entry_id to id. I think it makes sense. Although an entry_id may appear in a table multiple times, it still acts in the same way as a regular id would.

[nelsonic]

@RobStallion apologies if that indentation was confusing. 😕
I think your progress on insert is looking good. 👍
And separately I think we should consider having the id of the record named :id instead of :entry_id because I don't know if it's worth having the :entry_ part, unless you feel that that it helps people to understand that it's different from a standard phoenix schema :id ...? 🤔

[RobStallion]

In my current comment table, I have made cid the primary key. If I try to insert two duplicate comments I get the following error...

    test "inserting the same comment twice fails" do
      Repo.insert(%Comment{comment: "hi"})
      Repo.insert(%Comment{comment: "hi"})
    end

  1) test Repo.insert/2 - with Comment struct: inserting the same comment twice fails (AlogTest.InsertTest)
     test/insert_test.exs:13
     ** (Ecto.ConstraintError) constraint error when attempting to insert struct:

         * comments_pkey (unique_constraint)

     If you would like to stop this constraint violation from raising an
     exception and instead add it as an error to your changeset, please
     call `unique_constraint/3` on your changeset with the constraint
     `:name` as an option.

     The changeset has not defined any constraint.

     code: Repo.insert(%Comment{comment: "hi"})
     stacktrace:
       (ecto) lib/ecto/repo/schema.ex:689: anonymous fn/4 in Ecto.Repo.Schema.constraints_to_errors/3
       (elixir) lib/enum.ex:1314: Enum."-map/2-lists^map/1-0-"/2
       (ecto) lib/ecto/repo/schema.ex:674: Ecto.Repo.Schema.constraints_to_errors/3
       (ecto) lib/ecto/repo/schema.ex:276: anonymous fn/15 in Ecto.Repo.Schema.do_insert/3
       test/insert_test.exs:15: (test)

[nelsonic]

@RobStallion if the comment is identical and therefore the CI derived from the data is the same, then it should fail. We need to figure out how to return that error message to the viewer.

[RobStallion]

I have created the following two tests...

    test "inserting the same comment twice fails with changeset" do
      Repo.insert(%Comment{comment: "hi"})

      {atom, _changeset} =
        %Comment{}
        |> Comment.changeset(%{comment: "hi"})
        |> Repo.insert()

      assert atom == :error
    end

    test "inserting the same comment twice fails without changeset" do
      Repo.insert(%Comment{comment: "hi"})
      Repo.insert(%Comment{comment: "hi"})
    end

The first returns an error to the user, but the second one is causing the test to error and stop running. This is not behaviour that we want to happen in applications using the adapter. However it doesn't look like the adapter is the step returning the error to the user so I am not sure how we would resolve this issue.

Could this be an issue with the migration & schema set up I have used for testing?

[RobStallion]

Just caught up with @SimonLab @Danwhy on zoom about the results of testing, see comment on this pr.

The migration file created for the tests will be changed once the execute_ddl functionality is merged in as it handle creating the cid, entry_id and deleted columns (we can also manage adding timestamps() I assume).

e.g.

    create table(:comments, primary_key: false) do
      add(:cid, :string, primary_key: true)
      add(:entry_id, :string)
      add(:deleted, :boolean, default: false)
      add(:comment, :string)

      timestamps()
    end

will become

    create table(:comments, primary_key: false) do
      add(:comment, :string)
    end

We have realised however that users will needs to manually add these fields to the schema file still as we are not able to control this file through the adapter.

The schema for the above migration will need to look something like...

  @primary_key {:cid, :string, autogenerate: false}
  schema "comments" do
    field(:entry_id, :string)
    field(:deleted, :boolean, default: false)
    field(:comment, :string)
  end

  def changeset(comment_struct, attrs \\ %{}) do
    comment_struct
    |> cast(attrs, [:cid, :entry_id, :comment, :deleted])
    |> unique_constraint(:cid, name: :comments_pkey)
  end

This is because the schema file is 'handled' (couldn't think of a better word) by the Ecto.Schema module.

We can create macros in our alog module in the future that could override this functionality. This way users would only need to put use Alog.Schema at the top of their file and then they could have a file closer to...

  use Alog.Schema

  schema "comments" do
    field(:comment, :string)
  end

  def changeset(comment_struct, attrs \\ %{}) do
    comment_struct
    |> cast(attrs, [:comment])
  end

Although this would be AMAZING, I do not believe that it is a blocker. I think that we should focus on getting the adapter finished and integrated with auth. Once this is working we can then come back and add the macros needed to have a more "clean" schema file.

For now, when we use the insert function in our adapter we will need call the relevant changeset function beforehand, and we will need to ensure that the changeset function has unique_constraint(:cid, name: :comments_pkey) added.

[RobStallion]

I think that the only thing we need to think about and discuss further is how we want to handle inserting duplicate data.

I will open a separate issue and link back to this.