Insert #50
Insert #50
Conversation
|
@Danwhy I do not think that this PR is complete (at all) but I think that it might be in a place that it can be merged into the adapter branch. Please let me know if you have any thoughts on this. |
|
I think if we add some tests, we may get a better idea of how we want it to actually work/what makes sense with regards to duplicate data etc. You'll need to delete or comment out all the old tests that have |
did not delete for now as I thought we could use as a base
Insert testing
|
Testing the insert functions has 'brought to light' some things that we could (and maybe should) change in the future. Please see this comment for more info on this. |
RobStallion
requested review from
Danwhy and
SimonLab
and removed request for
SimonLab and
Danwhy
| 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, []) |
There was a problem hiding this comment.
We might be able to enhance this existing cid lookup later on. Maybe we could use a cache system to store all the current cid (with ETS)? If we can manage to not send another query to Postgres that would be ideal but it's working for now 👍
lib/alog.ex
Outdated
| defp insert_logic(adapter_meta, source, prefix, params, on_conflict, returning, opts) do | ||
| {kind, conflict_params, _} = on_conflict | ||
| {fields, values} = :lists.unzip(params) | ||
| sql = @conn.insert(prefix, source, fields, [fields], on_conflict, returning) |
There was a problem hiding this comment.
I'm not sure how/where the @conn module attribute is defined, I can't see it being done in the Alog module.
I think a @conn is defined before the adapter is being called but I think module attributes are scoped to the module and can't be use in other modules (didn't find any explanation on the scope of the module on the Elixir doc https://elixir-lang.org/getting-started/module-attributes.html)
@RobStallion do you know where it is defined?
There was a problem hiding this comment.
There was a problem hiding this comment.
ok makes sense, so to recap because Ecto.Adapters.SQL module is used in Alog, all the module attributes in the macro are accessible on Alog.
defmodule Alog do
use Ecto.Adapters.SQL,
and
defmacro __using__(opts) do
quote do
@behaviour Ecto.Adapter
@behaviour Ecto.Adapter.Migration
@behaviour Ecto.Adapter.Queryable
@behaviour Ecto.Adapter.Schema
@behaviour Ecto.Adapter.Transaction
opts = unquote(opts)
@conn __MODULE__.Connection
...
There was a problem hiding this comment.
@RobStallion looks good, just a question about the module attribute @conn
There was a problem hiding this comment.
After creating the table and running the migrations the tests are green 🎉
Thanks @RobStallion 👍
| end | ||
| end | ||
| end | ||
| # use Alog.TestApp.DataCase |
There was a problem hiding this comment.
when commenting out an entire file, consider adding a line at the top explaining why.
otherwise the next person reading it will scratch their heads and have to do forensics to understand it. 🤔
There was a problem hiding this comment.
@RobStallion insert is looking good. ✅
Please consider leaving a comment (preferably linking to an issue/discussion) explaining why several tests are commented out. 💬
Happy to merge into adapter branch to continue work. 👍
#45
"alog-ifies" insert functionality, adding a CID made from the params and a unique entry_id from a substring of the CID.
Adds tests for insert function
This step, as it is currently, allows for duplicate data to be inserted. Comments have been added to code for my thoughts on this matter.