feat: verify the database server at startup - #19
Conversation
Greptile SummaryThe PR adds non-blocking database compatibility checks to the doctor.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "fix: observe the server version once" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if minimum && server_version < minimum | ||
| reasons << "#{self.class.name.demodulize} #{server_version} is older than " \ | ||
| "Solid Objects requires, which is #{minimum}" |
There was a problem hiding this comment.
unsupported_server_reasons reads server_version separately for the comparison and warning text, and the doctor reads it again for a passing message. A transient failure on a later read replaces an already determined unsupported-version result with a generic verification error, while normal checks perform an avoidable extra database round trip; preserve one observed version for both the status and message.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/solid_objects/database_adapter.rb
Line: 54-56
Comment:
**Repeated server-version reads**
`unsupported_server_reasons` reads `server_version` separately for the comparison and warning text, and the doctor reads it again for a passing message. A transient failure on a later read replaces an already determined unsupported-version result with a generic verification error, while normal checks perform an avoidable extra database round trip; preserve one observed version for both the status and message.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Valid. The version was read for the comparison, again for the message, and a third time by the doctor for its passing text. Beyond the wasted round trips, a transient failure on a later read would have replaced an already determined unsupported-version result with a generic verification error, which is the worse half of the problem.
One observed version now flows through: the doctor reads it once and passes it to unsupported_server_reasons, which keeps its no-argument form for direct callers. A test asserts the doctor observes the version exactly once.
|
@greptileai review |
Each adapter now reports its version against the oldest server Solid Objects is exercised against, and MySQL additionally confirms Solid Objects tables use InnoDB, since a non-transactional engine would silently break fenced commits. The doctor reports this as database_server and warns rather than failing, because refusing to run on an untested server would be a worse failure than running on one. PostgreSQL reports a packed integer, so it is normalised: comparing 170010 directly would make a 9.6 server look newer than any minimum, which the PostgreSQL run caught.
The reasons list read the version for its comparison and again for its message, and the doctor read it a third time for the passing text. A transient failure on a later read would have replaced an already determined result with a generic verification error, and every healthy check paid extra round trips. One observed version now decides both status and message.
49f920b to
f4d302e
Compare
1 participant
Roadmap milestone 4.
What it adds
Each adapter reports
server_versionagainstminimum_server_version, the oldest server Solid Objects is actually exercised against: PostgreSQL 13, MySQL 8.0, SQLite 3.35. MySQL additionally verifies that Solid Objects tables use InnoDB, because a non-transactional engine would silently break fenced commits, which is the property the whole activation model rests on.The doctor surfaces this as a
database_servercheck.Warn, do not refuse
The check warns rather than failing, and an unsupported server does not make the report unhealthy. Refusing to boot on an untested server would be a worse failure than running on one: the minimums record what has been exercised, not what is known to break. Verification failures are also caught and reported rather than raised, so an unreadable version cannot take down a runtime role.
A bug the PostgreSQL run caught
PostgreSQL's
database_versionreturns a packed integer,170010for 17.10. Comparing that against13passes, but so would90600for PostgreSQL 9.6, making the check useless on the adapter it matters most for. The adapter now normalises it, and a test asserts the reported version is a human version rather than a packed integer, so this cannot regress silently.Tests
Ten tests: the version is reported and is not a packed integer, an old PostgreSQL server is still detected, a supported server passes, an old server is reported rather than raised, an unreadable version does not raise, the doctor passes and warns appropriately, an unsupported server does not fail the report, and the two MySQL InnoDB cases.
Verified against SQLite and a real PostgreSQL 17 locally; MySQL paths run in CI.
Roadmap
Milestone 4 removed and recorded under "Implemented and tested", with the remaining milestones renumbered, per the requirement in #15.
Compatibility
No database change, no migration, no initializer regeneration. Additive adapter methods. The doctor gains a check that can warn on servers it previously said nothing about; nothing fails that did not fail before.