[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
2 times, most recently
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.
gallone2000
force-pushed
the
alessandro_cre_1299_support_white_listed_requests
branch
from
8088491 to
f92a05b
Compare
gallone2000
force-pushed
the
alessandro_cre_1299_support_white_listed_requests
branch
from
f92a05b to
2ae6046
Compare
ReinierMaas
left a comment
ReinierMaas
left a comment
There was a problem hiding this comment.
LGTM! I have two small remaining notes but nothing blocking the merge.
| // When fetched from DB with no metadata rows, json_group_object returns '{}'. | ||
| let submission = Submission { | ||
| strategic_metadata: Default::default(), | ||
| ..submission | ||
| }; |
There was a problem hiding this comment.
The submission now always carries the strategic_metadata and it is by default the default so this should be unnecessary.
| match submission { | ||
| match submission_row { | ||
| None => Err(E::R(SubmissionNotFound(id))), | ||
| Some(submission) => Ok(submission), | ||
| 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.0, | ||
| otel_trace_carrier: row.otel_trace_carrier, | ||
| }), |
There was a problem hiding this comment.
Can we now with the AS "strategic_metadata!: sqlx::types::Json<StrategicMetadataMap>" switch back to the shorter query_as! without the manual mapping? Or is the row.strategic_metadata.0 the only syntax that prevents us from doing so?
No description provided.