Typed schema for better integrate with json column#162
Conversation
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fff3653b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def changed_in_place?(raw_old_value, new_value) | ||
| cast_value(raw_old_value) != new_value | ||
| end |
There was a problem hiding this comment.
When ActiveRecord checks whether a JSON attribute changed, it calls changed_in_place? with the raw DB value and the current (type-cast) value. Here cast_value(raw_old_value) always builds a new schema instance, and EasyTalk schema instances only implement == for Hash (otherwise they use object identity). That means two instances with identical data will still compare unequal, so the attribute is marked dirty on every save even if nothing changed. This breaks dirty tracking/partial updates for records that haven’t been modified.
Useful? React with 👍 / 👎.
| value = record.public_send(prop_name) | ||
| next if value.blank? || !value.is_a?(String) | ||
|
|
||
| Time.parse(value) | ||
| Time.zone.parse(value) | ||
| record.errors.add(prop_name, 'must be a valid time in HH:MM:SS format') unless value.match?(/\A\d{2}:\d{2}:\d{2}/) |
There was a problem hiding this comment.
Switching from Time.parse to Time.zone.parse introduces a crash when Time.zone is nil (common outside Rails or before time zone configuration). In that case Time.zone.parse raises NoMethodError, which isn’t rescued here, so validations blow up instead of adding an error. This regression affects ActiveModel-only usage or early initialization paths.
Useful? React with 👍 / 👎.
|
kindly ping @sergiobayona |
|
@jasl thanks! could you write a basic example showing how ActiveModelType would be used in a real scenario? |
I'm working on a hobby project https://github.com/jasl/tavern_kit/tree/main/playground which is a SillyTavern clone, and it's heavily using JSONB columns to store very complex but structured configurations, for example https://github.com/jasl/tavern_kit/blob/main/playground/app/models/conversation_settings/character_settings.rb Without this PR, I need to use
I need to handle its state myself. WIth the PR, no custom code needed, just |
Fallback to Time.parse when Time.zone is unset so time format validation adds errors instead of raising for ActiveModel-only usage.
Use a frozen BOOLEAN_VALUES constant instead of allocating [true, false] repeatedly during validation.
3 participants
Inspired by https://github.com/DmitryTsepelev/store_model
It helps model structured JSON columns.