Skip to content

Format s3 key #432

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lindseydew wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Format s3 key #432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3798bd3
Format s3 key from external id
lindseydew Aug 6, 2025
8112097
Format s3 key in the ingestion lambda post
lindseydew Aug 11, 2025
800b2c9
Run the queueing lambda
lindseydew Aug 11, 2025
dc7b6e4
Merge branch 'main' into ld/format-s3-key
lindseydew Aug 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion fingerpost-queueing-lambda/src/handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type { SQSBatchResponse, SQSEvent } from 'aws-lambda';
import { getFromEnv } from '../../shared/config';
import { formatS3Key } from '../../shared/formatS3key';
import { createLogger } from '../../shared/lambda-logging';
import { putToS3AndQueueIngestion } from '../../shared/putToS3AndQueueIngestion';
import { putToS3 } from '../../shared/s3';
Expand All @@ -13,7 +14,7 @@ export const main = async (event: SQSEvent): Promise<SQSBatchResponse> => {
const externalId = messageAttributes['Message-Id']?.stringValue;
const hasExternalId = externalId && externalId.trim().length > 0;
const objectKey = hasExternalId
? `${externalId}.json`
? formatS3Key(externalId)

@bryophyta bryophyta Aug 13, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Should the formatting be fingerpost-specific or apply to all items going via S3? If the latter, should we add this to the poller lambdas too?

@lindseydew lindseydew Aug 13, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good thought. Do we store the original id anywhere with the poller lambdas?

I felt like doing some manipulation here was safe because we have the externalId in the database to cross reference.

Whereas with the poller lambda I don't know whether we might not want to risk any additional manipulation? Though, I also think it's nicer to store the records in this way.

@andrew-nowak do you have any thoughts?

: `GuMissingExternalId/${sqsMessageId}.json`;

if (hasExternalId) {
Expand Down
7 changes: 6 additions & 1 deletion scripts/start
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ startDockerContainers() {
yes | ./db/flyway.sc migrate local
}

startQueueingLambda() {
pushd "$ROOT_DIR/fingerpost-queueing-lambda"
npm install
npm run dev
}
startIngestionLambda() {
pushd "$ROOT_DIR/ingestion-lambda"
npm install
Expand Down Expand Up @@ -100,7 +105,7 @@ main() {
echo "Using the local stack. Starting Docker, Ingestion Lambda and Play App."
startDockerContainers
# run all three, and terminate together when the user presses `ctrl+c`: https://stackoverflow.com/a/52033580
(trap 'kill 0' SIGINT; startIngestionLambda & startPlayApp & wait)
(trap 'kill 0' SIGINT; startQueueingLambda & startIngestionLambda & startPlayApp & wait)

@bryophyta bryophyta Aug 13, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤩

fi
}

Expand Down
19 changes: 19 additions & 0 deletions shared/formatS3key.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { formatS3Key } from "./formatS3key";

describe('formatS3Key', () => {
it('should format the S3 key with the external ID and .json extension', () => {
const externalId = 'test-external-id';
const expectedKey = 'test-external-id.json';
expect(formatS3Key(externalId)).toBe(expectedKey);
});
it('should replace spaces with a hyphen', () => {
const externalId = 'test external id';
const expectedKey = 'test-external-id.json';
expect(formatS3Key(externalId)).toBe(expectedKey);
});
it('should replace forward slashes with a hyphen', () => {
const externalId = 'test/external/id';
const expectedKey = 'test-external-id.json';
expect(formatS3Key(externalId)).toBe(expectedKey);
Comment on lines +5 to +17

@andrew-nowak andrew-nowak Aug 13, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this run the risk that some stories might overwrite each other in S3? if we had 3 files arrive with each of these external ids, then we'd be left with only the most recent, even though they've each got different ids. (Hopefully actually we still have all 3, but only as versions of object with the same key).
Do we have a concrete problem with the spaced or slashed keys? Maybe we run the id through a url encoder instead?

@lindseydew lindseydew Aug 14, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main thing with the spaced and slashed keys is we end up with a lot of documents with the key name as 0.json when the rest of the external id is stored as prefixes, which I was finding a bit confusing when I was trying to compare inputs.

Though yeah, I do get that doing our own manipulation could have some risks when we want to ensure the key is the identifier.

What would be the benefit of a URLEncoder instead?

@lindseydew lindseydew Aug 14, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you both think? A problem worth addressing or prefer to leave as is?

})
});;
3 changes: 3 additions & 0 deletions shared/formatS3key.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export function formatS3Key(externalId: string): string {
return externalId.replace(/\s+/g, '-').replace(/\//g, '-').concat('.json');
}
5 changes: 3 additions & 2 deletions shared/putToS3AndQueueIngestion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { getFromEnv } from './config';
import { getErrorMessage } from './getErrorMessage';
import { putToS3 } from './s3';
import { sqs } from './sqs';
import { formatS3Key } from './formatS3key';

export async function putToS3AndQueueIngestion({
externalId,
Expand All @@ -16,7 +17,7 @@ export async function putToS3AndQueueIngestion({
keyPrefix: string;
body: string;
}): Promise<{ status: 'success' } | { status: 'failure'; reason: string }> {
const objectKey = `${keyPrefix}/${externalId}.json`;
const objectKey = `${keyPrefix}/${formatS3Key(externalId)}`;

try {
const s3PutResult = await putToS3({
Expand Down Expand Up @@ -58,4 +59,4 @@ export async function putToS3AndQueueIngestion({
reason: `Error when trying to put to S3 and queue the ingestion Lambda: ${getErrorMessage(error)}.`,
};
}
}
}