gistfile1.txt

[8:46 AM]  
Caleb Thompson Is this the direction you were thinking? https://github.com/rails/rails/compare/master...calebthompson:null-constraint-errors-are-turned-into-validation-errors

[9:11 AM]  
Sean Griffin Almost, yeah. There should be a field on the `PG::NotNullViolation` error that references the column(s) that were violated. If it's not exposed, we should make a PR to the pg gem exposing it

[9:12 AM]  
(For unique constraint violations it's going to give the constraint name not the column name -- but I'd rather parse that than the error message)

[9:12 AM]  
I'm not sure if we should only have this occur if a class macro is called on a per-column basis or something

[9:12 AM]  
To start I'd like to have it be opt-in at least behind a config flag since it's technically a breaking change

[9:15 AM]  
Caleb Thompson There weren’t any methods exposing that sort of thing.

[9:15 AM]  
Sean Griffin Hm ok

[9:16 AM]  
Well this is the function that has it: https://www.postgresql.org/docs/9.5/static/libpq-exec.html#LIBPQ-PQRESULTERRORFIELD

[9:16 AM]  
And `PG_DIAG_COLUMN_NAME` is the field we need

[9:18 AM]  
It looks like there is something exposed

[9:18 AM]  
The `PG::Result` class is on the exception under the ivar `@result` (I'm assuming there's an attr_reader)

[9:18 AM]  
And then https://github.com/ged/ruby-pg/blob/e5cb1df93c496560444941d7a531e4cd675ec7c2/ext/pg_result.c#L399-L413 looks like it's on result

[9:19 AM]  
As the method `error_field`

[9:19 AM]  
Ah actually there's examples for exactly what you need in the docs for that method

[9:20 AM]  
Caleb Thompson oh look.

[9:20 AM]  
I’ll check and see if there’s similar stuff for other dbs.

[9:20 AM]  
I assume I’ll need to implement this across the board.

[9:21 AM]  
Oh shit it’s going to break for non rails/rails dbs too.

[9:21 AM]  
This may get done eventually.

[9:23 AM]  
Sean Griffin So I think this should be done in a way that if a DB supports it, it's sugar on top

[9:23 AM]  
And if a DB doesn't support it, nothing breaks

[9:23 AM]  
Easiest way to ensure we implement it that way is to not implement this for SQLite (which will literally make us parse error messages to do anything useful here and I'd prefer to do as little of that as possible)

[9:24 AM]  
Caleb Thompson
```e.cause.result.error_field(PG::Result::PG_DIAG_COLUMN_NAME)
 => "event_id" ```

[9:24 AM]  
works

[9:25 AM]  
I guess if I do nothing these’ll keep getting raised as they do now.

[9:26 AM]  
Sean Griffin Yeah ideally we can normalize these into something that can be handled slightly higher up

[9:26 AM]  
Caleb Thompson That’s what my first error does bro.

[9:26 AM]  
Pushed that change

[9:26 AM]  
Sean Griffin I guess I was thinking even more general but fair point

[9:27 AM]  
So yeah if the adapter doesn't normalize the error for us, the behavior is the same as before this patch. I think that's fine

[9:28 AM]  
Caleb Thompson I’ll spike on implementing it for other dbs that won’t require string parsing and explain why I didn’t do it for dbs that do.

[9:29 AM]  
Sean Griffin :+1:

[9:29 AM]  
Caleb Thompson But with this as POC, should I hit the rails-contrib list or just do a PR when I’m ready?

[9:29 AM]  
Sean Griffin And if we can implement this in a way that there's just one or two places that we do a global `rescue StatementInvalid` and then ask the adapter to try to normalize it, that would be nice as well

[9:29 AM]  
Caleb Thompson Not sure I follow that.

[9:30 AM]  
Sean Griffin Like somewhere in `Relation`

[9:30 AM]  
Caleb Thompson Bounce the error back to the adapter for the validation messages?

[9:30 AM]  
Sean Griffin
```rescue ActiveRecord::StatementInvalid => e
  if connection.normalize_handlable_error(e.original_exception)
    # do stuff
  else
    raise
  end
end
```

[9:31 AM]  
More so that we don't have to remember to do this in `exec_insert` and also `exec_update` and also whatever else for every adapter

[9:31 AM]  
And the abstract adapter just has a default implementation that returns `nil`

[9:32 AM]  
Caleb Thompson and that returns something like `[[:event_id, :blank], [:user_id, :unique]]` that I populate to the model or something else?

[9:32 AM]  
Sean Griffin And then the only remaining question on this for me is whether we want something like `database_uniqueness_validation :foo`, or something like `self.convert_database_errors_into_active_model_errors = true` or something else entirely

[9:33 AM]  
Caleb Thompson If it’s a macro it’s not worth doing.

[9:33 AM]  
Sean Griffin Maybe it even would be ok to just do this by default (probably not, but we could come close by having a global setting which is `true` for new apps)

[9:33 AM]  
Yeah I like your proposal for what it returns

[9:33 AM]  
Caleb Thompson Because it could be a validation now.

[9:34 AM]  
The benefit is in catching stuff developers miss. A config makes more sense.

[9:34 AM]  
Sean Griffin yeah

[9:34 AM]  
I almost want to turn this into a generic thing where we give the validator objects a chance to handle these after they've been normalized but that's probably too much

[9:37 AM]  
Caleb Thompson `create_or_update` makes sense to me as a place to handle this I think. Maybe even a module that `AR::Validations` includes into `AR::Persistence` that wraps the behavior and rescues specific exception types.

[9:37 AM]  
Sean Griffin Yeah I like that

[9:38 AM]  
Caleb Thompson
```def create_or_update(*args, &block)
  super
rescue ActiveRecord::NotNull => e
  # someone adds some errors
rescue ActiveRecord::NotUnique => e
  # someone adds some errors
rescue ActiveRecord::ForeignKeyWhatever => e
```

[9:38 AM]  
instead of the `connection.normalize_handlable_error` stuff

[9:38 AM]  
Sean Griffin Yeah but then the connection has to normalize in multiple places

[9:38 AM]  
Caleb Thompson right yes.

[9:39 AM]  
Sean Griffin We can probably discuss this stuff on the PR. It'll be easier to talk about with the full context

[9:39 AM]  
Caleb Thompson so
```def create_or_update(*args, &block)
  begin
    super
  rescue StatementInvalid
    raise connection.normalize_handlable_error(e.original_exception)
  end
rescue
rescue
rescue
end```

[9:40 AM]  
Sean Griffin Yeah that's one way to structure it

[9:40 AM]  
The return value doesn't necessarily need an exception object though

[9:40 AM]  
I'd just go with what you think is best for now. This is super nitpicky stuff. :slightly_smiling_face:

[9:40 AM]  
Caleb Thompson Subsequent rescues don’t catch previous re-raises do they?

[9:41 AM]  
sure

[9:41 AM]  
Sean Griffin The way you set that up I think it would but I'm not 100% cerain

[9:41 AM]  
certain*

[9:42 AM]  
Caleb Thompson
```2.3.0 :010 > def raiser
2.3.0 :011?>   raise StandardError
2.3.0 :012?>   rescue => e
2.3.0 :013?>   puts 'first'
2.3.0 :014?>   raise
2.3.0 :015?>   rescue => e
2.3.0 :016?>   puts 'second'
2.3.0 :017?>   end
 => :raiser 
2.3.0 :018 > raiser
first
StandardError: StandardError
        from (irb):11:in `raiser'
        from (irb):18
        from /Users/caleb/.rvm/rubies/ruby-2.3.0/bin/irb:11:in `<main>'
```

[9:43 AM]  
 ```2.3.0 :019 > def raiser
2.3.0 :020?>   begin
2.3.0 :021 >       raise StandardError
2.3.0 :022?>     rescue => e
2.3.0 :023?>     puts 'first'
2.3.0 :024?>     raise ArgumentError
2.3.0 :025?>     end
2.3.0 :026?>   rescue => e
2.3.0 :027?>   puts 'second'
2.3.0 :028?>   end
 => :raiser 
2.3.0 :029 > raiser
first
second
 => nil 
```

[9:43 AM]  
`begin` works.

[9:43 AM]  
OK goodbye