Repository navigation
Sync upstream with gradual SQL view conversion and compatible error tools - #42
Conversation
* fix: install script make installer platform aware, shell profile safe * coderabbit comments fix * coderabbit comments fix * add separator newline when missing
* Misc updates - added query param `?format=compact` to get a compact schema (defaults to JSON) - if severity text/number is missing then the value is derived using the other field if present * remove nullable from compact schema
* add log, trace errors as tools * deesource error fix * get_dataset_schema tool to return compact TSV response
currently failing because UI sends `stream_name=sql` in create/update filters when called from sql editor the stream name validation was added in the PR parseablehq#1727 for a security fix GHSA-q8xx-r33r-6mwv with this change, the security fix still works also, UI will be able to save the filter as-is
![comment-clanker[bot]](https://avatars.githubusercontent.com/u/338781737?s=60&v=4){kind=small}
| start_time: required_string(arguments, "startTime")?.to_owned(), | ||
| end_time: required_string(arguments, "endTime")?.to_owned(), |
There was a problem hiding this comment.
[P2] Preserve the requested time window when aggregating log errors.
query_internal parses these bounds with TimeRange::parse_human_time, which truncates both endpoints to the minute. The new aggregate query contains no exact timestamp predicate. A request for 12:00:30Z–12:01:30Z therefore includes errors from the preceding 30 seconds and drops errors in the final 30 seconds; a range entirely inside one minute becomes empty. This also affects ranges produced by resolve_time_range, whose end includes seconds.
Parse and validate the exact bounds, apply an explicit half-open timestamp predicate before aggregation (using the dataset's configured timestamp column), and expand the transport bounds to cover that exact interval. Add a regression fixture with rows before, inside, and at/after the requested boundaries, including a subminute range.
There was a problem hiding this comment.
Fixed in commit 85358dff (pushed). The log-error tool now uses exact half-open SQL bounds and a covering minute-aligned transport range. Filtering and lastSeen use the configured timestamp column; relative endpoints share one clock value.
TDD: the new DataFusion regression first failed because an error before the requested start was included. After the fix, all 21 tool tests pass, including six cases covering default/custom timestamps, cross-minute/subminute/fractional ranges, both boundaries, and the final partial minute. Formatting and strict library Clippy also pass. Fresh Sol 6.1 and Opus 5.5 reviews of the pushed fix are underway.
| // IDs are normally globally unique. Fail closed for an administrator | ||
| // if imported records make this owner-less URL ambiguous. | ||
| if matches.next().is_some() { | ||
| return None; |
There was a problem hiding this comment.
[P2] Preserve access to an administrator's own copy when legacy IDs collide.
The previous admin-update implementation could leave two records with the same filter ID under different owners. This PR now preserves both records in the cache, but an admin matching either owner is denied all operations because both match this predicate. Prefer an exact owner match before the admin fallback; keep rejecting an ambiguous fallback for unrelated admins. Also distinguish (owner, ID) in the native Saved views selector: it currently uses only filter_id for option keys/values and selects the first match, making the second owner's view inaccessible. Keep both owners' data intact and add upgrade/collision regression coverage.
There was a problem hiding this comment.
Fixed in e57d1ed7 (pushed). Cache lookup prefers the requester's own record before requiring a unique administrator fallback. The UI uses owner + ID for selection and preserves the original ID in API requests; no owners' records are deleted or combined.
RED: the administrator's own copy returned None, and the browser selected the other owner's view. GREEN: 21 filter tests and all 27 Logs browser tests pass, including independent selection/apply/update/conversion/deletion of colliding views.
| PARSEABLE | ||
| .metastore | ||
| .reconcile_filter(&filter, &tenant_id) | ||
| .await?; |
There was a problem hiding this comment.
[P2] Do not fail a new-view creation on unrelated alias cleanup.
The server has already durably written and cached a newly generated ULID here. Reconciliation then probes that new ID in every existing dataset directory for the owner. An unreadable unrelated directory or transient storage error returns a failure after the view was created. The UI does not refresh on error, and retrying creates another view with a new ID. New IDs have no old aliases to reconcile; reserve alias cleanup for updates/conversion/deletion. A failing-storage regression should assert that an unrelated lookup cannot turn a successful new write into a failed POST.
There was a problem hiding this comment.
Fixed in e57d1ed7 (pushed). New-filter POST no longer performs alias reconciliation for its freshly generated ID; update/conversion/deletion retain reconciliation.
RED: the live HTTP regression created an unreadable sibling directory and observed HTTP 500 after exactly one new JSON record was persisted. GREEN: the same test against the rebuilt binary succeeds, together with the complete authorization, conversion, stale-update, reconciliation and delete/restart smoke test.
| onOpenChange={(nextOpen) => { | ||
| if (!nextOpen) { | ||
| generation.current += 1; | ||
| setBusy(false); |
There was a problem hiding this comment.
[P2] Keep the mutation lock while a save is still pending.
Start a slow Save new view, close the dialog, and reopen before POST completes. This resets busy even though the request is still running, enabling another Save and a second POST. The first response then fails the generation check and skips the list refresh. Preserve the in-flight mutation state across close/reopen, refresh the current dataset after completion, and keep suppressing stale selection/notice updates. Add a browser regression that holds POST pending across close/reopen and verifies only one create request occurs.
There was a problem hiding this comment.
Fixed in e57d1ed7 (pushed). A mutation token keeps writes locked across close/reopen, refreshes after a successful current-context write, and prevents old dataset responses from unlocking a newer save. Dialog generations still protect selection and draft text. Failed conversion cleanup keeps Retry available.
RED: the browser regression observed Save enabled while the first POST was pending. GREEN: all 27 Logs browser tests and 74 focused unit tests pass, including close/reopen, dataset-switch races and cleanup-after-write retry. TypeScript, production build, formatting and strict Rust library Clippy pass. Fresh Sol 6.1 and Opus 5.5 reviews are checking this pushed commit.
There was a problem hiding this comment.
Repeated Sol 6.1 and Opus 5.5 reviews are clean on e57d1ed70ea870da0a1cf971076f8b9e16d0db83: no remaining actionable PR findings in the latest independent passes.
Four confirmed findings were posted as resolvable threads, reproduced with failing tests, fixed in 85358dff / e57d1ed7, and followed up with RED/GREEN evidence: precise log-error windows, legacy cross-owner ID collisions, new-filter failure after persistence, and duplicate saves during dialog close/reopen. Review of the UI fix also added coverage for conversion cleanup retry and dataset changes during pending writes.
Validation: 21 tool tests, 21 filter tests, 27 Logs Chromium tests, 74 focused frontend unit tests, nine installer tests, rebuilt-server HTTP/restart regression, TypeScript/production build, formatting and strict library Clippy pass. The latest CI checks are still running; this review does not claim they have passed.
The reviewed operational constraints remain documented: filter mutation locking is within one query-server process, and older native UIs cannot display converted views. No deployment or merge was performed.
3 participants
Sync upstream through
a5976d2ewhile keeping native saved log views usable. An unchanged upstream merge would rewrite their dataset association tosql, leave old persisted copies behind, and change the existing schema tool response type.Changes
stream_name: "sql". New native views stay dataset scoped until their owner selects Convert to SQL view. Converted views remain attached to their original explorer dataset, and subsequent edits preserve their format.POST /api/v1/filters/{id}/convert-to-sql. Preserve owner, ID, query, time filter and extra metadata, including on admin updates and stale-client edits.format.Validation
The additional
cargo clippy --lib --tests -- -D warningscheck reports existing test-only lints in unchangedknown_schema.rs,handlers/http/mod.rs,staging/reader.rs, andstreams.rs. Strictcargo clippy --lib -- -D warningspasses.Operational notes
Reconciliation runs at startup and after mutations; there is no one-time flag that can conceal partial cleanup. Mutations are serialized within one query-server process, not across multiple independent query servers sharing storage. An older native UI cannot display converted views; retain this compatible UI during rollback. See conversion and recovery notes. Existing deployments were not modified during testing.
Need help on this PR? Tag
@codesmith-botwith what you need.