rbi:models generates a String type sig instead of T.nilable(String)

[benjamin-thomas]

Describe the bug:

Hi,

This project looks promising, thanks.

I'm barely dipping my toes into the sorbet world so forgive me if I'm misunderstanding something.

Executing the following command generates wrong column definitions:

bundle exec rake rails_rbi:models[Family]

I'm using PostgreSQL server 13.1

Steps to reproduce:

I'm developing an app in the open so I could provide a link if required (the setup is docker based). Otherwise the following screenshots should make things clear I think.

I have 2 tables backed by an activerecord model. Each of them, have the same column name "name"

However, I made a mistake and forgot to declare name NON NULL on the families table upon the first migration.

This is the types of bug inducing errors I'd like to catch with sorbet-rails.

Expected behavior:

The generated signature does not reflect the nullable nature of the column.

Instead, I have to override to with my own definition

Versions:

• Ruby: Rails 6.1.2.1
• Rails: Rails 6.1.2.1
• Sorbet: sorbet (0.5.6287)
• Sorbet-Rails: sorbet-rails (0.7.3)

---

But it seems all the info is available to the runtime though, so the signature should be correctly generated. Is this normal behavior?

I prefer to deal with structure.sql dumps, but I also generated a schema.rb file to ensure that wouldn't be a problem (none of those files seem to be used by the gem though).

Thanks for your feedback.

[benjamin-thomas]

Ok so this is "by design" since you seem to check the model validation at this method call

I'd argue that this approach is not correct, since:

• a NULL value could be set by other processes outside of the webapp if the db allows it and the webapp needs to be aware of it.
• also, some NULL values have probably been stored and will need to be dealt with at some point
• not handling them will probably result in a runtime error, thus defeating the purpose.

Would you be okay to allow overriding this behavior via an env var for instance?

[hdoan741]

@benjamin-thomas Yeah, I agree this behavior can/should be configurable. We have a config object. I think we can add an option there.

[benjamin-thomas]

@manhhung741, I'd like to look at this again in the next coming days.

Would you say it'd be okay to have the defaults being "strict", and revert to the old (lax) behavior via a config entry (I would print an appropriate warning to STDERR)