Skip to content

Commit c3a8602

Browse files
NogaNHSNogaNHS
authored
[PRMP-625] Set S3 key in documentReference model (#828)
1 parent f6f2c5d commit c3a8602

File tree

5 files changed

+161
-4
lines changed

5 files changed

+161
-4
lines changed

‎lambdas/models/document_reference.py‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ class DocumentReference(BaseModel):
7979
)
8080
nhs_number: str
8181
s3_bucket_name: str = Field(exclude=True, default=None)
82-
s3_file_key: str = Field(exclude=True, default=None)
82+
s3_file_key: str = Field(default=None)
8383
status: Literal["current", "superseded", "entered-in-error"] = Field(
8484
default="current"
8585
)
@@ -107,11 +107,14 @@ def set_location_properties(cls, data, *args, **kwargs):
107107
file_location = data.get("file_location") or data.get("FileLocation")
108108
bucket, key = cls._parse_s3_location(file_location)
109109
data["s3_bucket_name"] = bucket
110-
data["s3_file_key"] = key
110+
if "s3_file_key" not in data and "S3FileKey" not in data:
111+
data["s3_file_key"] = key
111112
elif "s3_bucket_name" in data:
112-
data["s3_file_key"] = cls._build_s3_key(data)
113+
current_s3_file_key = cls._build_s3_key(data)
114+
if "s3_file_key" not in data:
115+
data["s3_file_key"] = current_s3_file_key
113116
data["file_location"] = cls._build_s3_location(
114-
data["s3_bucket_name"], data["s3_file_key"]
117+
data["s3_bucket_name"], current_s3_file_key
115118
)
116119
return data
117120

‎lambdas/services/upload_document_reference_service.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,7 @@ def update_dynamo_table(
220220
"virus_scanner_result",
221221
"doc_status",
222222
"file_location",
223+
"s3_file_key",
223224
"file_size",
224225
"uploaded",
225226
"uploading",

‎lambdas/tests/unit/models/test_document_reference.py‎

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,151 @@ def test_last_updated_within_three_minutes_return_false_when_last_updated_is_mor
5151
assert expected == actual
5252

5353

54+
def test_set_location_properties_with_file_location_sets_bucket_and_key():
55+
data = {
56+
"id": "test-id-123",
57+
"file_name": "test.pdf",
58+
"nhs_number": "9000000009",
59+
"file_location": "s3://my-bucket/folder/9000000009/test-id-123",
60+
}
61+
62+
doc_ref = DocumentReference.model_validate(data)
63+
64+
assert doc_ref.s3_bucket_name == "my-bucket"
65+
assert doc_ref.s3_file_key == "folder/9000000009/test-id-123"
66+
assert doc_ref.file_location == "s3://my-bucket/folder/9000000009/test-id-123"
67+
68+
69+
def test_set_location_properties_with_FileLocation_pascal_case():
70+
data = {
71+
"id": "test-id-456",
72+
"file_name": "test.pdf",
73+
"nhs_number": "9000000009",
74+
"FileLocation": "s3://another-bucket/path/to/file",
75+
}
76+
77+
doc_ref = DocumentReference.model_validate(data)
78+
79+
assert doc_ref.s3_bucket_name == "another-bucket"
80+
assert doc_ref.s3_file_key == "path/to/file"
81+
assert doc_ref.file_location == "s3://another-bucket/path/to/file"
82+
83+
84+
def test_set_location_properties_with_s3_bucket_name_builds_location():
85+
data = {
86+
"id": "test-id-789",
87+
"file_name": "test.pdf",
88+
"nhs_number": "9000000009",
89+
"s3_bucket_name": "test-bucket",
90+
}
91+
92+
doc_ref = DocumentReference.model_validate(data)
93+
94+
assert doc_ref.s3_bucket_name == "test-bucket"
95+
assert doc_ref.s3_file_key == "9000000009/test-id-789"
96+
assert doc_ref.file_location == "s3://test-bucket/9000000009/test-id-789"
97+
98+
99+
def test_set_location_properties_with_s3_bucket_and_subfolder():
100+
data = {
101+
"id": "test-id-101",
102+
"file_name": "test.pdf",
103+
"nhs_number": "9000000009",
104+
"s3_bucket_name": "test-bucket",
105+
"sub_folder": "patients/archive",
106+
}
107+
108+
doc_ref = DocumentReference.model_validate(data)
109+
110+
assert doc_ref.s3_bucket_name == "test-bucket"
111+
assert doc_ref.s3_file_key == "patients/archive/9000000009/test-id-101"
112+
assert (
113+
doc_ref.file_location
114+
== "s3://test-bucket/patients/archive/9000000009/test-id-101"
115+
)
116+
117+
118+
def test_set_location_properties_with_s3_bucket_subfolder_and_doc_type():
119+
data = {
120+
"id": "test-id-202",
121+
"file_name": "test.pdf",
122+
"nhs_number": "9000000009",
123+
"s3_bucket_name": "test-bucket",
124+
"sub_folder": "patients/active",
125+
"doc_type": "ARF",
126+
}
127+
128+
doc_ref = DocumentReference.model_validate(data)
129+
130+
assert doc_ref.s3_bucket_name == "test-bucket"
131+
assert doc_ref.s3_file_key == "patients/active/ARF/9000000009/test-id-202"
132+
assert (
133+
doc_ref.file_location
134+
== "s3://test-bucket/patients/active/ARF/9000000009/test-id-202"
135+
)
136+
137+
138+
def test_set_location_properties_with_explicit_s3_file_key():
139+
data = {
140+
"id": "test-id-303",
141+
"file_name": "test.pdf",
142+
"nhs_number": "9000000009",
143+
"s3_bucket_name": "test-bucket",
144+
"s3_file_key": "custom/path/to/file",
145+
}
146+
147+
doc_ref = DocumentReference.model_validate(data)
148+
149+
assert doc_ref.s3_bucket_name == "test-bucket"
150+
assert doc_ref.s3_file_key == "custom/path/to/file"
151+
assert doc_ref.file_location == "s3://test-bucket/9000000009/test-id-303"
152+
153+
154+
def test_set_location_properties_file_location_does_not_override_explicit_s3_key():
155+
data = {
156+
"id": "test-id-404",
157+
"file_name": "test.pdf",
158+
"nhs_number": "9000000009",
159+
"file_location": "s3://bucket-one/old/path/file",
160+
"s3_file_key": "new/path/file",
161+
}
162+
163+
doc_ref = DocumentReference.model_validate(data)
164+
165+
assert doc_ref.s3_bucket_name == "bucket-one"
166+
assert doc_ref.s3_file_key == "new/path/file"
167+
168+
169+
def test_set_location_properties_with_leading_slash_in_key():
170+
data = {
171+
"id": "test-id-505",
172+
"file_name": "test.pdf",
173+
"nhs_number": "9000000009",
174+
"s3_bucket_name": "test-bucket",
175+
"s3_file_key": "/leading/slash/path",
176+
}
177+
178+
doc_ref = DocumentReference.model_validate(data)
179+
180+
assert doc_ref.file_location == "s3://test-bucket/9000000009/test-id-505"
181+
182+
183+
def test_set_location_properties_complex_s3_location():
184+
data = {
185+
"id": "test-id-606",
186+
"file_name": "test.pdf",
187+
"nhs_number": "9000000009",
188+
"file_location": "s3://prod-bucket/year/2024/month/10/patient/9000000009/test-id-606",
189+
}
190+
191+
doc_ref = DocumentReference.model_validate(data)
192+
193+
assert doc_ref.s3_bucket_name == "prod-bucket"
194+
assert doc_ref.s3_file_key == "year/2024/month/10/patient/9000000009/test-id-606"
195+
assert (
196+
doc_ref.file_location
197+
== "s3://prod-bucket/year/2024/month/10/patient/9000000009/test-id-606"
198+
)
54199
def test_infer_doc_status_returns_deprecated_when_deleted():
55200
MOCK_DOCUMENT_REFERENCE.deleted = "2023-10-30T10:25:00.000Z"
56201
MOCK_DOCUMENT_REFERENCE.uploaded = True

‎lambdas/tests/unit/services/test_pdf_stitching_service.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,7 @@ def test_migrate_multipart_references(mock_service):
320320
"ID": f"{TEST_DOCUMENT_REFERENCES[0].id}",
321321
"LastUpdated": 1704110400,
322322
"NhsNumber": f"{TEST_DOCUMENT_REFERENCES[0].nhs_number}",
323+
"S3FileKey": f"{TEST_DOCUMENT_REFERENCES[0].s3_file_key}",
323324
"Status": "current",
324325
"Uploaded": True,
325326
"Uploading": False,
@@ -340,6 +341,7 @@ def test_migrate_multipart_references(mock_service):
340341
"ID": f"{TEST_DOCUMENT_REFERENCES[1].id}",
341342
"LastUpdated": 1704110400,
342343
"NhsNumber": f"{TEST_DOCUMENT_REFERENCES[1].nhs_number}",
344+
"S3FileKey": f"{TEST_DOCUMENT_REFERENCES[1].s3_file_key}",
343345
"Status": "current",
344346
"Version": "1",
345347
"Uploaded": True,
@@ -360,6 +362,7 @@ def test_migrate_multipart_references(mock_service):
360362
"ID": f"{TEST_DOCUMENT_REFERENCES[2].id}",
361363
"LastUpdated": 1704110400,
362364
"NhsNumber": f"{TEST_DOCUMENT_REFERENCES[2].nhs_number}",
365+
"S3FileKey": f"{TEST_DOCUMENT_REFERENCES[2].s3_file_key}",
363366
"Status": "current",
364367
"Version": "1",
365368
"Uploaded": True,
@@ -438,6 +441,7 @@ def test_write_stitching_reference(mock_service, mock_uuid):
438441
"ID": f"{TEST_UUID}",
439442
"LastUpdated": TEST_1_OF_1_DOCUMENT_REFERENCE.last_updated,
440443
"NhsNumber": f"{TEST_1_OF_1_DOCUMENT_REFERENCE.nhs_number}",
444+
"S3FileKey": f"{TEST_NHS_NUMBER}/{TEST_UUID}",
441445
"FileSize": 8000,
442446
"Status": "current",
443447
"Uploaded": True,
@@ -586,6 +590,7 @@ def test_rollback_reference_migration(mock_service):
586590
"ID": f"{TEST_DOCUMENT_REFERENCES[0].id}",
587591
"LastUpdated": 1704110400,
588592
"NhsNumber": f"{TEST_DOCUMENT_REFERENCES[0].nhs_number}",
593+
"S3FileKey": f"{TEST_DOCUMENT_REFERENCES[0].s3_file_key}",
589594
"Status": "current",
590595
"Version": "1",
591596
"Uploaded": True,
@@ -607,6 +612,7 @@ def test_rollback_reference_migration(mock_service):
607612
"ID": f"{TEST_DOCUMENT_REFERENCES[1].id}",
608613
"LastUpdated": 1704110400,
609614
"NhsNumber": f"{TEST_DOCUMENT_REFERENCES[1].nhs_number}",
615+
"S3FileKey": f"{TEST_DOCUMENT_REFERENCES[1].s3_file_key}",
610616
"Status": "current",
611617
"Version": "1",
612618
"Uploaded": True,
@@ -628,6 +634,7 @@ def test_rollback_reference_migration(mock_service):
628634
"ID": f"{TEST_DOCUMENT_REFERENCES[2].id}",
629635
"LastUpdated": 1704110400,
630636
"NhsNumber": f"{TEST_DOCUMENT_REFERENCES[2].nhs_number}",
637+
"S3FileKey": f"{TEST_DOCUMENT_REFERENCES[2].s3_file_key}",
631638
"Status": "current",
632639
"Version": "1",
633640
"Uploaded": True,

‎lambdas/tests/unit/services/test_upload_document_reference_service.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -387,6 +387,7 @@ def test_update_dynamo_table_clean_scan_result(service, mock_document_reference)
387387
"doc_status",
388388
"file_location",
389389
"file_size",
390+
"s3_file_key",
390391
"uploaded",
391392
"uploading",
392393
},

0 commit comments

Comments
 (0)