-
Notifications
You must be signed in to change notification settings - Fork 44
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.
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
Courey/create synonym server only #1933
Courey/create synonym server only #1933
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember that the terms that have been passed to the function may themselves already have synonyms, and so you either need to:
- Link all of their synonyms with a common UUID, or
- Decide (and document) that this function will unlink any mutual synonyms of those terms that you do not pass to it.
See, for example,
gcn.nasa.gov/app/lib/synonyms.server.ts
Lines 115 to 154 in cadd4e4
export async function addSynonym(...terms: string[]) { | |
const db = await tables() | |
const table = db.synonyms | |
const tableName = db.name('synonyms') | |
const doc = db._doc as unknown as DynamoDBDocument | |
// Get the unique set of UUIDs that need to be updated | |
const { Responses } = await doc.batchGet({ | |
RequestItems: { | |
[tableName]: { | |
Keys: terms.map((term) => ({ term })), | |
ProjectionExpression: 'uuid', | |
}, | |
}, | |
}) | |
const uuidsToUpdate: string[] = Array.from( | |
new Set(Responses?.[tableName]?.map(({ uuid }) => uuid) ?? []) | |
) | |
// Get the unique set of terms that need to be updated | |
const { Items } = await table.query({ | |
IndexName: 'synonymsByUuid', | |
KeyConditionExpression: 'uuid IN :uuid', | |
ExpressionAttributeValues: { ':uuid': uuidsToUpdate }, | |
ProjectionExpression: 'term', | |
}) | |
const termsToUpdate = Array.from( | |
new Set([terms, ...Items.map(({ term }) => term)]) | |
) | |
// Get the new UUID. Reuse an existing UUID in the set if possible. | |
const uuid = uuidsToUpdate[0] ?? crypto.randomUUID() | |
// Update all of the terms. | |
await doc.batchWrite({ | |
RequestItems: { | |
[tableName]: termsToUpdate.map((term) => ({ term, uuid })), | |
}, | |
}) | |
} |
The current behavior:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I question whether all of this validation is necessary. You could just as easily document that the function expects, as a precondition, that none of the terms that you pass have existing synonyms.
is the why behind:
if you wanted me to do it the second way, why didn't you just tell me to document it? |
That's what I was trying to say with the second option I proposed:
Note that if you do go this route, then you don't need to add a function to unlink synonyms, because that's a degenerate case of the same behavior. |
So one concern I have with the just linking it to another synonym is that is can leave synonym groups of 1 in its wake. Is that acceptable or should there then be additional complexity to remove the synonym entirely if it only has one remaining synonym? |
An isolated synonym is fine. |
1412b88
to
ecd6d35
Compare
10a37bb
to
b831727
Compare
Title
feat: add create function to synonym server
Description
This function takes an array of eventIds that are synonymous to each other and creates a grouping of synonym records.
Reviewers
Leo - because he's already reviewed it before i finished this comment.
Changes
This adds a function that creates synonyms based off eventIds passed in. It returns an array of synonym records that were created.
Acceptance Criteria
a user can pass an array of eventIds in and it will create the expected synonym records for each eventId.