[CRE-1299] - support white listed requests#113
Conversation
There was a problem hiding this comment.
Pull request overview
This PR exposes per-submission strategic metadata end-to-end so that “white listed requests” (likely keyed off strategic metadata) can be represented on the Submission object and surfaced through the Python bindings.
Changes:
- Added
strategic_metadata: Option<StrategicMetadataMap>to the core RustSubmissiontype and initialized it in constructors. - Updated submission DB reads (
get_submission,submission_status) to fetch strategic metadata viasubmissions_metadata(usingjson_group_object) and normalize empty metadata toNone. - Extended the
opsqueue_pythonSubmissionbinding to includestrategic_metadataand include it in__repr__.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| opsqueue/src/common/submission.rs | Adds strategic_metadata to Submission and populates it from DB queries via submissions_metadata aggregation. |
| libs/opsqueue_python/src/common.rs | Surfaces strategic_metadata in the Python Submission wrapper and updates its __repr__. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn normalize_strategic_metadata( | ||
| strategic_metadata: Option<sqlx::types::Json<StrategicMetadataMap>>, | ||
| ) -> Option<StrategicMetadataMap> { | ||
| strategic_metadata.and_then(|json| (!json.0.is_empty()).then_some(json.0)) | ||
| } |
gallone2000
force-pushed
the
alessandro_cre_1299_support_white_listed_requests
branch
from
b8151e3 to
f5bed41
Compare
gallone2000
force-pushed
the
alessandro_cre_1299_support_white_listed_requests
branch
from
f5bed41 to
5692b0c
Compare
ReinierMaas
left a comment
ReinierMaas
left a comment
There was a problem hiding this comment.
LGTM! I have two optional extensions, one is removing the type level distinction that doesn't actually exist. I think that would make the whole API easier to use and understand!
ReinierMaas
left a comment
ReinierMaas
left a comment
There was a problem hiding this comment.
I require a few more changes.
There was a problem hiding this comment.
What is this file? Is it a alternative storage format for the assert_snapshot! tests? We don't have the same for the tests in strategy.rs.
| , ( SELECT json_group_object(metadata_key, metadata_value) | ||
| FROM submissions_metadata | ||
| WHERE submission_id = submissions.id | ||
| ) AS "strategic_metadata: sqlx::types::Json<StrategicMetadataMap>" | ||
| , otel_trace_carrier | ||
| FROM submissions WHERE id = $1 | ||
| "#, | ||
| id | ||
| ) | ||
| .fetch_optional(conn.get_inner()) | ||
| .await?; | ||
| match submission_row { | ||
| None => Err(E::R(SubmissionNotFound(id))), | ||
| Some(row) => Ok(Submission { | ||
| id: row.id, | ||
| prefix: row.prefix, | ||
| chunks_total: row.chunks_total, | ||
| chunks_done: row.chunks_done, | ||
| chunk_size: row.chunk_size, | ||
| metadata: row.metadata, | ||
| strategic_metadata: row.strategic_metadata.map(|json| json.0), | ||
| strategic_metadata: row | ||
| .strategic_metadata | ||
| .map(|json| json.0) | ||
| .unwrap_or_default(), |
There was a problem hiding this comment.
I think unwrap_or_default can be prevented by adding an exclamation mark at 479:
AS "strategic_metadata!: sqlx::types::Json<StrategicMetadataMap>"
All of these functions return a valid JSON or JSONB object or array, even if there are no valid input rows. ~ https://sqlite.org/json1.html#jgroupobject
There shouldn't be any cases where json_group_object doesn't return a valid JSON object.
| chunk_size, | ||
| metadata, | ||
| strategic_metadata: None, | ||
| strategic_metadata: strategic_metadata.clone(), |
There was a problem hiding this comment.
This clone can be prevented as insert_submission below now can rely on the strategic_metadata travelling with the submission. Internally it only uses references to the data so duplicating it can be prevented completely.
No description provided.