[Log Entry] Inital Migration And Code To Specificy A Log Entries Schema #172
Conversation
CulmoneY
requested review from
Copilot and
pranavrao145
and removed request for
Copilot
CulmoneY
force-pushed
the
yianni/add-log-schema
branch
from
aea28ae to
b039e3a
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a log schema feature for projects, allowing custom attributes to be defined and validated for log entries. The implementation includes database migration, model methods for managing schema attributes, validation logic, and test coverage.
Key Changes:
- Added
log_schemaJSON column to projects table with default empty hash - Implemented
add_log_attributeandremove_log_attributemethods with validation for three types: numerical, boolean, and text - Updated seed data to include default log schema for all projects
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| db/migrate/20260106211202_add_log_schema_to_projects.rb | Migration adding log_schema text column with JSON serialization |
| db/schema.rb | Schema update reflecting the new log_schema column |
| app/models/project.rb | Model implementation with attribute definition, validation, and helper methods |
| test/models/project_test.rb | Test coverage for add/remove attribute operations and validation |
| db/seeds/projects.rb | Updated seed data to include default log schema for all seeded projects |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def log_schema_shape | ||
| log_schema.each do |key, value| | ||
| unless VALID_LOG_SCHEMA_TYPES.include?(value) | ||
| errors.add(:log_schema, "Invalid log schema: #{key} has invalid type #{value}") | ||
| end | ||
| end |
There was a problem hiding this comment.
The validation method doesn't check if log_schema keys are strings or if the log_schema is a Hash. If log_schema is not a Hash or contains non-string keys or values, calling .each could raise an error or behave unexpectedly.
| def add_log_attribute(name, type) | ||
| schema = log_schema.dup | ||
| schema[name] = type | ||
| update!(log_schema: schema) | ||
| end |
There was a problem hiding this comment.
The method doesn't validate that the type parameter is valid before adding it to the schema. This could allow invalid types to be added, which would only be caught when save! is called. Consider validating the type parameter upfront or handling the validation error.
| def add_log_attribute(name, type) | ||
| schema = log_schema.dup | ||
| schema[name] = type | ||
| update!(log_schema: schema) | ||
| end | ||
|
|
||
| def remove_log_attribute(name) | ||
| schema = log_schema.dup | ||
| schema.delete(name) | ||
| update!(log_schema: schema) | ||
| end |
There was a problem hiding this comment.
The public methods add_log_attribute and remove_log_attribute lack documentation explaining their parameters, purpose, and potential exceptions (e.g., ActiveRecord::RecordInvalid). Consider adding documentation for these API methods.
| test "#add_log_attribute adds a new attribute to the log schema" do | ||
| project = create(:project) | ||
| project.add_log_attribute("temperature", "numerical") | ||
| assert_equal({ "temperature" => "numerical" }, project.log_schema) | ||
| end | ||
|
|
||
| test "#remove_log_attribute removes an attribute from the log schema" do | ||
| project = create(:project, log_schema: { "humidity" => "numerical", "status" => "text" }) | ||
| project.remove_log_attribute("humidity") | ||
| assert_equal({ "status" => "text" }, project.log_schema) | ||
| end | ||
|
|
||
| test "invalid log schema raises validation error" do | ||
| project = build(:project, log_schema: { "invalid_attr" => "unsupported_type" }) | ||
| assert_not_predicate project, :valid? | ||
| end |
There was a problem hiding this comment.
The test coverage is missing several important scenarios: testing add_log_attribute with an invalid type, testing behavior when log_schema is not a Hash, testing that add_log_attribute overwrites existing keys, and testing edge cases like empty string keys or nil values.
TL;DR
Briefly describe what this PR does and why it is needed. Include any relevant background or context.
While its not best practice to write a migration, and use that added column in the same PR, since we have no deployment database this operation is completely safe.
The next PR will add the ability to edit these files.
Checklist