Use nullable field from changes

[chiroptical]

This text is broken up into two pieces. The first is the context for the fix (which I think is correct) and the second is a question because I am misunderstanding something. I also want to try to add some tests for this.

Fix

I wanted a migration that adds a NOT NULL constraint to an existing field. You can see the example file in the question section below. This didn't work. After a little print debugging I realized that the column.nullable meant the existing column which would retain its' nullability always. You could never remove or add it with the TOML. I am pretty sure this works for adding or removing a nullability constraint but I want to write some tests which I'll do soon!

Question

The next issue I faced is given the following migration,

[[actions]]
type = "alter_column"
table = "users"
column = "name"

  [actions.changes]
  nullable = false

After reshape migrate, I am unable to add new rows to the schema. I am a bit out of my element here and still learning, but I tried the following,

insert into migration_5_make_name_not_null.users (name) values ('chiroptical');

which yields,

null value in column "name" of relation "users" violates not-null constraint
DETAIL:  Failing row contains (8, null, null).

I am a bit confused here. Am I supposed to only insert data into the public schema and let the triggers handle everything else?
How does this work with SET search_path TO migration_5_make_name_not_null? Maybe I need to read more postgresql documentation here and I shouldn't be asking you?

[fabianlindfors]

Hi, Barry!

You're absolutely right that this is broken. I have a test for this but apparently it's badly written and doesn't check to make sure the constraint holds for the new schema...

Your fix looks good but I think it should be something like this: self.changes.nullable.unwrap_or(column.nullable). If the existing column is not nullable then the temporary column shouldn't be either. I have a fix almost ready for this with some improved tests.

Regarding your other question, all updates and inserts should happen with the schema path set. This way, the updates and inserts will be run against the view rather than the underlying table. This works because of a cool Postgres feature called updatable views.

It's of course possible to edit the table directly but then Reshape will assume you are using the old schema (and you should also avoid touching any temporary columns). The general idea with Reshape though is that all interactions happen against views thanks to the schema path. The application doesn't know this though as updatable views work basically exactly like tables!

[chiroptical]

Your fix looks good but I think it should be something like this: self.changes.nullable.unwrap_or(column.nullable). If the existing column is not nullable then the temporary column shouldn't be either. I have a fix almost ready for this with some improved tests.

💯 this is exactly right. I'll try the schema set command before the insert next time and read up on updateable views. Feel free to close this PR. Is this working okay for you? Me opening up a PR/issue on a topic and you responding? I don't want to be too much of a burden.

[fabianlindfors]

You're the opposite of a burden! I'm excited to have somebody using Reshape, pointing out bugs and any confusing things. I really appreciate it. As I mentioned on the previous PR, your first livestream helped me find a bunch of improvements.

Keep it up (if you want)! :)

[fabianlindfors]

Fixed with 7cc4f23, thank you! Improved the existing test to actually check the constraint and also added a new test for making a NOT NULL column nullable. Took the opportunity to add an example to the README as well, making a column NOT NULL feels like a common operation :)