Skip to content

Fix nil FK queries, strict_where validation, field name conflict warnings#348

Closed
sawirricardo wants to merge 1 commit into
active-hash:masterfrom
sawirricardo:fix/low-hanging-issues-302-190-184
Closed

Fix nil FK queries, strict_where validation, field name conflict warnings#348
sawirricardo wants to merge 1 commit into
active-hash:masterfrom
sawirricardo:fix/low-hanging-issues-302-190-184

Conversation

@sawirricardo

Copy link
Copy Markdown

Summary

Three low-hanging fixes addressing open issues.

Fixes #302 — belongs_to with nil foreign key makes useless query

When accessing a belongs_to association with a nil foreign key, ActiveHash was still calling find_by_* which triggers a DB query. ActiveRecord skips this query when the key is nil.

Fix: Early return nil when foreign key is nil in:

  • ActiveRecordExtensions#belongs_to
  • belongs_to_active_hash
  • Methods#belongs_to
  • Methods#has_one

No breaking change — nil key previously returned nil anyway after a useless query.

Fixes #190where(undefined_key: nil) returns all records

where() with an undefined field silently returns all records because read_attribute returns nil for unknown keys, and nil == nil matches everything.

Fix: Opt-in strict_where class attribute. When enabled:

class Country < ActiveHash::Base
  self.strict_where = true
  fields :name, :code
end

Country.where(typo_key: nil)
# => ArgumentError: Unknown field(s): typo_key. Valid fields are: id, name, code

Also validates against data keys for models that use undeclared fields via data=. Opt-in only — no breaking change.

Fixes #184field :display silently returns nil

Rails adds display to Object. ActiveHash skips defining the method if one already exists, but gives no indication. Users get silent nil returns and confusing test failures.

Fix: warn() in validate_field when field name conflicts with an existing method:

class TestHash < ActiveHash::Base
  field :display
  # => warning: ActiveHash: Field :display on TestHash conflicts with an existing method...
end

Additive warning only — no breaking change.

Test Results

276 specs pass, 2 pre-existing sqlite3 adapter failures unrelated to these changes.

…onflicts

Fixes active-hash#302: belongs_to/has_one no longer issue queries when foreign key is nil.
Early return nil instead of calling find_by_* with nil, matching AR behavior.

Fixes active-hash#190: Add opt-in strict_where class attribute. When enabled, where()
raises ArgumentError for unknown field names, catching typos like
where(typo_key: nil) that silently returned all records.

Fixes active-hash#184: Warn when field name conflicts with an existing method
(e.g. :display added to Object by Rails). Previously silently returned nil.
@flavorjones

Copy link
Copy Markdown
Collaborator

@sawirricardo Thanks very much for opening this PR!

However, if this is three individual fixes, we would prefer it to be broken out into three separate pull requests, so each change can be reviewed and merged on its own.

Also, I would like to see tests added that clearly describe the problem being fixed. If you need any help figuring out to craft an appropriate test, I'm happy to help -- just drop a note here!

I appreciate your time -- thanks for understanding how we like to work. I hope you're able to make time to contribute in the future! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants