upsert_person/1

[nelsonic]

Getting a foreign_key_constraint when running this on a fresh laptop:

INSERT INTO "items" ("person_id","status_code","text","inserted_at","updated_at") VALUES ($1,$2,$3,$4,$5) RETURNING "id" [194400, 2, "test item", ~N[2022-07-31 03:35:50], ~N[2022-07-31 03:35:50]]
↳ AppWeb.AppLive.handle_event/3, at: lib/app_web/live/app_live.ex:33
[error] GenServer #PID<0.783.0> terminating
** (Ecto.ConstraintError) constraint error when attempting to insert struct:

    * items_person_id_fkey (foreign_key_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 `foreign_key_constraint/3` on your changeset with the constraint
`:name` as an option.

The changeset has not defined any constraint.

The person record is being inserted into Postgres as:

For clarity: the problem with this picture is that the same person (me) is being inserted into the db multiple times when I login using multiple web browsers ... 🤦‍♂️

Todo:

• upsert the person with the id matching their jwt.id so that when they login again they can see their items.

[SimonLab]

I'm looking at this issue at the moment. I'll try to replicate the error first

Start from scratch to make sure my application is clean:

• rm -rf _build to avoid any tailwind error, see mix setup fail #95 (comment)
• mix setup This will drop, create and run the seed file on Postgres. After this command a person is created on the database to represent "guest" users (people not logged in yet)
• Login with Github. I spot already an error, as two people (the same I guess) are created on login. I'm not sure this is directly related to the main foreign key issue yet.
• Create a new item, error occurs!

I'm going to reset the database with mix ecto.drop and mix ecto.create to make sure the errors are still there

• Still multiple users are created. Also another person is inserted when the item creation fails

The user is inserted each time the LiveView is mounted or disconnected:
https://github.com/dwyl/app-mvp/blob/67d62bb9fc04092a3a9a58bd54cfe0324ab248f5/lib/app_web/controllers/auth_controller.ex#L5-L20

On line 12 we see App.Person.create(claims). We need to make sure to use upsert here, or only insert the person if the id is not defined yet.

[SimonLab]

Only insert new person

• We are using auth for authentication. On the auth app a person is created when the user loggedin (if it doesn't exist yet)
• on this app we have also the people table and at the moment a new person is created each time we check for the jwt session
• The jwt contains the id value created on the auth app, this id is unique to the person, see Can we rely on ID to stay constant? auth_plug#23.

I propose that we had the auth_id field on the people table with a unique constraint. This field will allow us to check if the person already exist on the application and use upsert to avoid creating duplicate. It will also allow us to retreive the person from the database and assign the people id (instead of the auth id) and to make sure we don't have anymore foreign key constraint errors.

[SimonLab]

PR #116 should fix this issue

[nelsonic]

Hi @SimonLab, thanks for taking a look at this. I had assigned it to myself the other day and started working on it but didn't push my code because some tests were failing. Please see: c77d43a for the implementation I had in mind.
No new fields, just assigning the jwt.id to the person.id.

[SimonLab]

I thought about using the id of the people table and assign to it the jwt.id values. My reason to create the new auth_id field was to avoid creating conflicts. Ecto creates automatically the id field as a primary key and auto increment it by default.

We can define manually our primary key when the table is created on the migration:

• We first define the option primary_key to false in the table function to not generate the default id field, see https://hexdocs.pm/ecto_sql/Ecto.Migration.html#table/2
• Add manually the id field with the primary_key option to true, see https://hexdocs.pm/ecto_sql/Ecto.Migration.html#add/3

  def change do
    create table(:people, primary_key: false) do
     add(:id, :integer, primary_key: true)
      add(:givenName, :binary)
      add(:auth_provider, :string)
      add(:key_id, :integer)
      add(:picture, :binary)
      add(:locale, :string)
      add(:status_code, :integer)

      timestamps()
    end
  end

We can then continue to use the upsert feature of Postgres, see https://hexdocs.pm/ecto/constraints-and-upserts.html#upserts and only insert or update the person fields when necessary.

Ultimately because we are using the auth app and the people table is mirrored in the app-mvp application without adding more information to the people table, I think we can completely remove the people table and instead have just the people_id field in the items table and removing the foreign key.

[nelsonic]

Yeah, Ecto will create the auto-incrementing PK. 👍
Hence the work-around for testing purpose: https://github.com/dwyl/app-mvp/pull/90/files#r938585420

If the person.id comes from auth via the JWT and we insert it there should be zero conflicts.
Basically the only way to create a new person in the MVP and that's via the auth_controller.ex
https://github.com/dwyl/app-mvp/pull/90/files#r938586551
So the PK conflict should never arise. 💭

Agree (in principal) with removing the people table. That would be a good simplification. 💡
However from the perspective of making the MVP self-explanatory and having the ERD #89 (comment) that explains how the data is structured ... I think I still prefer to keep it. Don't you? (for learning purposes ...) 🤔

[nelsonic]

BTW: The implementation on my branch works fine without any further code. 💭

[SimonLab]

If we explain the id for people comes from the auth app in the Readne I think it's ok to remove the people table. However I can also see it can be useful to keep it to explain how the items and person are related in the database, I let you choose which one you prefer.

What bothered me on the people table is the auto-increment, I'll check what happen when we insert a value in a autoincrement field. But yes agree that this shouldn't be an issue when using the jwt.id.

However I think we could remove the set_id function.
Because the attrs will always have the id defined (from auth), we don't need to create a new request to Postgres to count the number of people in the table.

[nelsonic]

Yeah, the set_id/1 was only there for testing. 🙄
It was a trade-off between complicating the tests vs. the implementation. ⚖️
And I wish I had just created a person_fixture/1 helper function with a random id instead. 💭

Definitely going to remove the people table #118 ✂️
thanks again for the insightful pre-review. 👌