revert: back out sqlite data updates#469
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (37)
📝 WalkthroughWalkthroughThis PR updates JVM distribution metadata across five JSON files covering Early Access and General Availability channels on multiple platforms. Changes normalize timestamps, standardize the ChangesJVM Metadata Refresh
Postgres migration and tooling
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Greptile SummaryThis PR reverts the JVM metadata JSON files from #467 back to the pre-SQLite baseline (removing malformed
Confidence Score: 3/5The data revert is clean, but the new PostgreSQL repository layer has two functional defects that will cause silent data-correctness problems in production. The upsert WHERE clause uses src/db/jvm_repository.rs — upsert NULL-comparison logic and features empty-string handling; src/db/pool.rs — Important Files Changed
Reviews (2): Last reviewed commit: "Revert "fix(db): use sqlite for data upd..." | Re-trigger Greptile |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| gunzip -c {{arg(name="file")}} | \ | ||
| psql postgresql://$ROAST_DB_HOST:$ROAST_DB_PORT/$ROAST_DB_NAME \ | ||
| --username=$ROAST_DB_USR \ | ||
| --password |
There was a problem hiding this comment.
db restore prompts password
Medium Severity
The new db:restore task passes psql --password, which forces an interactive password prompt, so restores fail or hang when run non-interactively (typical for mise tasks) even when ROAST_DB_PWD is configured in .env.local.
Reviewed by Cursor Bugbot for commit 794ea79. Configure here.
jdx
force-pushed
the
codex/revert-update-data-467
branch
from
794ea79 to
3f251dc
Compare
jdx
changed the title
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3f251dc. Configure here.
| rayon = "1" | ||
| regex = "1" | ||
| reqwest = { version = "0.13", features = ["blocking", "gzip", "json", "zstd"] } | ||
| rusqlite = { version = "0.39.0", features = ["chrono", "bundled"] } |
There was a problem hiding this comment.
SQLite stack removed unintentionally
High Severity
This change swaps the project back to PostgreSQL (ROAST_DATABASE_URL, rusqlite/r2d2_sqlite removed, CI import dropped) even though the PR states only JVM JSON from #467 should be reverted and SQLite from #465 should stay.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 3f251dc. Configure here.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| excluded.architecture != JVM.architecture | ||
| OR excluded.checksum != JVM.checksum | ||
| OR excluded.checksum_url != JVM.checksum_url | ||
| OR excluded.features != JVM.features | ||
| OR excluded.file_type != JVM.file_type | ||
| OR excluded.filename != JVM.filename | ||
| OR excluded.image_type != JVM.image_type | ||
| OR excluded.java_version != JVM.java_version | ||
| OR excluded.jvm_impl != JVM.jvm_impl | ||
| OR excluded.os != JVM.os | ||
| OR excluded.release_type != JVM.release_type | ||
| OR excluded.size != JVM.size | ||
| OR excluded.url != JVM.url | ||
| OR excluded.vendor != JVM.vendor | ||
| OR excluded.version != JVM.version |
There was a problem hiding this comment.
PostgreSQL evaluates NULL != 'value' as NULL (not TRUE), so any row where a nullable column (checksum, checksum_url, features, size) transitions between NULL and a non-NULL value will silently skip the update. For example, if a record exists with checksum = NULL and new data arrives with checksum = 'abc123', the condition excluded.checksum != JVM.checksum evaluates to NULL, which is falsy, and the SET clause never fires. The original SQLite code correctly used IS NOT (NULL-safe); the PostgreSQL equivalent is IS DISTINCT FROM.
| features: row | ||
| .get::<_, Option<String>>("features") | ||
| .map(|f| f.map(|f| f.split(',').map(String::from).collect()))?, | ||
| file_type: row.get("file_type")?, | ||
| filename: row.get("filename")?, | ||
| image_type: row.get("image_type")?, | ||
| java_version: row.get("java_version")?, | ||
| jvm_impl: row.get("jvm_impl")?, | ||
| os: row.get("os")?, | ||
| release_type: row.get("release_type")?, | ||
| size: row.get::<_, Option<i32>>("size")?, | ||
| url: row.get("url")?, | ||
| vendor: row.get("vendor")?, | ||
| version: row.get("version")?, | ||
| .map(|f| f.split(',').map(String::from).collect()), |
There was a problem hiding this comment.
features empty-string round-trip produces [""] on read-back
In map_workaround, Some(vec![]).join(",") produces Some(""). When that empty string is stored and later read back, "".split(',').collect() yields vec![""], not an empty vec. This recreates the exact features: [""] corruption the PR description says was fixed. The fix is to return an empty vec when the stored string is empty.
| features: row | |
| .get::<_, Option<String>>("features") | |
| .map(|f| f.map(|f| f.split(',').map(String::from).collect()))?, | |
| file_type: row.get("file_type")?, | |
| filename: row.get("filename")?, | |
| image_type: row.get("image_type")?, | |
| java_version: row.get("java_version")?, | |
| jvm_impl: row.get("jvm_impl")?, | |
| os: row.get("os")?, | |
| release_type: row.get("release_type")?, | |
| size: row.get::<_, Option<i32>>("size")?, | |
| url: row.get("url")?, | |
| vendor: row.get("vendor")?, | |
| version: row.get("version")?, | |
| .map(|f| f.split(',').map(String::from).collect()), | |
| features: row | |
| .get::<_, Option<String>>("features") | |
| .map(|f| if f.is_empty() { vec![] } else { f.split(',').map(String::from).collect() }), |
| } | ||
| } | ||
| let tls_connector = MakeTlsConnector::new(connector.build()); | ||
| let manager = PostgresConnectionManager::new(url.parse().unwrap(), tls_connector); |
There was a problem hiding this comment.
url.parse().unwrap() will panic on a malformed connection string
The URL already passed the starts_with("postgres://") guard, but parse() can still fail for other reasons (invalid port number, malformed host, unknown parameters, etc.). Using .unwrap() turns a user-configuration mistake into a hard panic instead of a clean error propagated through the Result return type.
| let manager = PostgresConnectionManager::new(url.parse().unwrap(), tls_connector); | |
| let manager = PostgresConnectionManager::new(url.parse()?, tls_connector); |
1 participant
Summary
Why
#467 imported already-exported JSON through the SQLite path and published malformed
features: [""]entries. Backing out #465 too returns the update pipeline to the previous database behavior while we prepare a cleaner fix.Validation
cargo fmt --checkpublic/api/jvm/**/*.jsonfiles contain zerofeatures == [""]rowspublic/api/jvm/ea/linux/x86_64.jsonis back to 4092 rows with no OpenJDK 28 rowsThis PR was generated by an AI coding assistant.
Note
High Risk
Switches the core database backend and CI secrets wiring while rewriting large published API datasets—any misconfiguration or partial revert affects consumers of
public/api/jvm.Overview
Replaces the SQLite persistence stack with PostgreSQL end-to-end: dependencies move from
rusqlite/r2d2_sqlitetopostgres,postgres-openssl, andr2d2_postgres;config.tomland docs now useROAST_DATABASE_URLand optional SSL settings instead of a localroast.sqlite3path;roast.sqlite3is dropped from.gitignore.The Update Data workflow no longer deletes a SQLite file or runs
cargo run -- import—it only **fetch**es againstROAST_DATABASE_URLfrom secrets. README adds Docker PostgreSQL bootstrap viasql/schema.sqland removes the JSON import seeding flow; mise gainsdb:dump/db:restorehelpers.public/api/jvm/**JSON is rolled back from the bad #467 export:created_atvalues return to prior 2025-era timestamps (instead of uniform2026-06-10), row ordering shifts, and entries no longer carry the malformed empty-stringfeaturespattern from that update.Reviewed by Cursor Bugbot for commit 3f251dc. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Chores
New Features
Refactor