refactor: decouple Record from Deposition with source-agnostic model#107
Merged
Conversation
Replace deposition_srn + indexes on Record with a discriminated union RecordSource (DepositionSource, HarvestSource) + convention_srn. Records can now originate from any pathway without hardwiring to depositions. Key changes: - RecordSource discriminated union in shared kernel (domain/shared/model/source.py) - RecordDraft value object as input to publish_record() - HookDefinition confined to validation boundary — downstream events carry only expected_features: list[str], not OCI runtime specs - Events tightened: files_dir/staging_dir removed from all domain events, storage adapters resolve paths from source identifiers - HookInputs simplified: deposition_srn replaced with run_id + files_dir - FeatureStoragePort.get_hook_output_root() resolves paths from source type + id, handler passes resolved path to FeatureService - IndexRef and find_by_deposition() removed, find_by_source() added - Alembic migration: drop deposition_srn/indexes, add source (JSONB) + convention_srn with functional unique index Closes #103
|
Contributor
Greptile SummaryThis PR replaces the tight Key changes:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| server/osa/domain/shared/model/source.py | Introduces the RecordSource discriminated union (DepositionSource |
| server/migrations/versions/source_agnostic_records.py | Adds the source (JSONB) and convention_srn columns, drops deposition_srn/indexes. Contains a down_revision vs docstring Revises: mismatch (add_device_authorizations vs consumer_group_delivery) that could create two migration heads. |
| server/osa/domain/record/model/draft.py | New RecordDraft value object cleanly encapsulates the inputs to RecordService.publish_record(), carrying source, metadata, convention_srn, and expected_features. |
| server/osa/domain/record/event/record_published.py | Replaces deposition_srn/hooks/files_dir with source: RecordSource, convention_srn: ConventionSRN (now required, not Optional), and expected_features: list[str]; the non-null convention_srn tightens the contract. |
| server/osa/domain/feature/handler/insert_record_features.py | Resolves hook_output_dir from the storage port at runtime (via source.type + source.id) instead of reading it from the event payload; delegates cleanly to FeatureService. |
| server/osa/infrastructure/persistence/mappers/record.py | Uses a module-level TypeAdapter(RecordSource) to deserialize the JSONB source column; _source_adapter.dump_python(…, mode="json") correctly returns a JSON-serializable dict for JSONB insertion. |
| server/osa/infrastructure/k8s/naming.py | label_value correctly generalised to accept `str |
| server/osa/infrastructure/persistence/tables.py | Table definition updated to match migration: source JSONB + convention_srn Text, with a globally-unique index on (source->>'type', source->>'id'). |
Sequence Diagram
sequenceDiagram
participant DS as DepositionService
participant VH as ValidateDeposition
participant VS as ValidationService
participant AH as AutoApproveCuration
participant CH as ConvertDepositionToRecord
participant RS as RecordService
participant IH as InsertRecordFeatures
participant FS as FeatureStoragePort
DS->>VH: DepositionSubmittedEvent(hooks, convention_srn)
VH->>VS: run_validation(hooks)
Note over VS: Extracts run_id from deposition_srn<br/>Resolves files_dir via hook_storage
VS-->>VH: ValidationRun + HookResults
VH->>AH: ValidationCompleted(expected_features=[h.name])
Note over VH: HookDefinitions confined here ─<br/>only feature names leak downstream
AH->>CH: DepositionApproved(expected_features, convention_srn)
CH->>RS: publish_record(RecordDraft(DepositionSource, metadata, expected_features))
RS-->>IH: RecordPublished(source, convention_srn, expected_features)
IH->>FS: get_hook_output_root(source.type, source.id)
FS-->>IH: hook_output_dir
IH->>FS: hook_features_exist(hook_output_dir, feature_name)
IH->>FS: read_hook_features(hook_output_dir, feature_name)
Reviews (2): Last reviewed commit: "refactor: update RecordPublished event c..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+56
to
+61
| def get_hook_output_root(self, source_type: str, source_id: str) -> str: | ||
| """Resolve the root directory for a given source type and id.""" | ||
| if source_type == "deposition": | ||
| srn = DepositionSRN.parse(source_id) | ||
| return str(self._dep_dir(srn)) | ||
| raise ValueError(f"Unknown source type: {source_type}") |
Contributor
There was a problem hiding this comment.
ValueError at runtime
Both FilesystemStorageAdapter.get_hook_output_root and S3StorageAdapter.get_hook_output_root raise ValueError(f"Unknown source type: {source_type}") for any source type other than "deposition". Because HarvestSource is now a first-class RecordSource type, any RecordPublished event emitted for a harvest-sourced record will cause InsertRecordFeatures.handle to crash at the get_hook_output_root("harvest", ...) call — before any feature storage path is even looked up.
The test TestInsertRecordFeaturesHarvestSource masks this because it mocks the storage:
storage.get_hook_output_root.return_value = "/fake/harvest/dir"The same gap exists in S3StorageAdapter (server/osa/infrastructure/s3/storage.py, line 181).
Either add a concrete path layout for harvest sources in both adapters, or guard feature insertion with a no-op path for source types whose storage layout is not yet defined (e.g. return an empty-features path and warn instead of raising).
Remove convention_srn from unique constraint and drop source_type index to simplify database schema. Remove unused find_by_source method from repository interface and implementation. Delete empty value objects file.
… convention fields Add ConventionSRN import and replace deposition_srn parameter with source and convention_srn fields in make_record_published helper function to align with updated event structure
Contributor
Author
|
@greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
deposition_srn+indexeson Record with a discriminated unionRecordSource(DepositionSource, HarvestSource) +convention_srn, so records can originate from any pathwayHookDefinitionto the validation boundary — downstream events carry onlyexpected_features: list[str], not OCI runtime specsFeatureStoragePort.get_hook_output_root()HookInputs: replacedeposition_srnwithrun_id+files_dirTest plan