feat: add functions to interact with relationships#764
Conversation
…event exposing unvalidated server action
…fic fields for a pub
tefkah
requested review from
3mcd,
allisonking and
kalilsn
and removed request for
allisonking and
kalilsn
| export type User = z.infer<typeof User>; | ||
|
|
||
| // Json value types taken from prisma | ||
| export type JsonObject = { [Key in string]?: JsonValue }; |
There was a problem hiding this comment.
this was needed bc we cast one to the other sooo frequently.
| }); | ||
| }; | ||
|
|
||
| export const updatePub = async ({ |
There was a problem hiding this comment.
i did change how this works, see individual commits/compare with old _updatePub. mainly the upserts are not by id, rely on the unique index
| oc | ||
| // we have a unique index on pubId and fieldId where relatedPubId is null | ||
| .columns(["pubId", "fieldId"]) | ||
| .where("relatedPubId", "is", null) |
There was a problem hiding this comment.
very nice, i tried this before but couldn't get it to work since i was specifying the name of the index. i now know that you have to specify the columns and condition for postgres to find a partial index in an on conflict statement!
There was a problem hiding this comment.
yeah that took me a second to figure out!
3mcd
left a comment
3mcd
left a comment
There was a problem hiding this comment.
Looks good other than a few small suggestions! Awesome job with the tests!
| includeRelations: boolean; | ||
| } = ({} = {}) | ||
| ) => autoCache(_getPubFields(props)); | ||
| export const getPubFields = (props: PubFieldsInput) => autoCache(_getPubFields(props)); |
There was a problem hiding this comment.
The _ convention is starting to confuse me a bit. I think we should just stick to more descriptive/verbose names because it's becoming unclear what _ means, e.g. is it a private function that is only exported for testing purposes? Or is it an uncached function that should be called carefully?
There was a problem hiding this comment.
I agree, I'm not a huge fan of it either. I think you're first interpretation is correct, i.e. a private function that's usually not exported, only done so for testing purposes. But it's indeed somewhat unclear, we've also used it in the past for _updatePub, with the idea that the underscored version was the "non server action" version. This was incorrect though, as it was still exported from a file with use server, making it a server action as well.
I think a more descriptive name is probably better. We could still use _ to indicate "this function should probably not be used outside of this module", but the function could be something like _uncachedGetPubFields.
| import { db } from "~/kysely/database"; | ||
| import { autoCache } from "./cache/autoCache"; | ||
|
|
||
| type PubFieldsInput = |
There was a problem hiding this comment.
Let's rename this GetPubFieldsInput to closer match its purpose.
| * @deprecated | ||
| */ | ||
| export const validatePubValuesBySchemaName = ({ | ||
| export const __validatePubValuesBySchemaName = ({ |
There was a problem hiding this comment.
i've updated the function name to be more clear about what i meant.
maybe i should just replace the usage of it everywhere instead? It doesn't do the exact same as the validatePubValuesBySchemaName though, hence why I hesitated to do this.
This function did a bit too much imo and had clunky usage for the caller. It both validated that the passed values match the schemas of the fields passed, but also that the values are only of the SchemaNames of those in the fields. This means that the caller needs to do a lot of weird matching and preprocessing which is very prone to error.
In the new function, you need to pass the schemaname together with the value it applies to, so this matching problem does not occur. However this does remove some functionality, namely the "making sure the values are only of the schemanames specified in the passed fields". It is on the caller to fix this before calling this function. Maybe we could provide a similar helper function specifically for this purpose, but I didn't want to make this PR even larger, so I just left the old function in and renamed it to signal we should fix this at some point.
| * Validates that all provided slugs exist in the community. | ||
| * @throws Error if any slugs don't exist in the community | ||
| */ | ||
| const consolidateFieldSlugs = async <T extends { slug: string }>({ |
There was a problem hiding this comment.
We may be able to come up with a better verb than "consolidate" for this function. "Hydrate" might be more accurate since the function takes existing objects with at least a slug property and adds (immutably) some new field-related properties.
Having said that I'm not sure I really like the approach here and instead I think this function could take an array or set of slugs and just return the field info for each slug. I don't think we're getting much by making this a generic that hydrates the objects with some new properties. Just overcomplicates it IMO.
There was a problem hiding this comment.
hmm i agree that this function might be doing too much, but i was motivated by trying to avoid the pitfalls of eg the old validatePubValues function(s). Because in that function, the field info is sort of decoupled from the value info, it's on the caller to mix and match them appropriately. With this function, you don't need to worry about that as the caller, you just supply the pubvalues and get the thing that you want, namely the hydrated pubvalues (which are necessary for validating said pubvalues).
Do you feel that maybe the new validation fn I wrote (see below) should work differently as well, e.g. it should not take an array of "hydrated" pubvalues with their field info? Because if you (like me) think that this new fn is easier to use, I think it makes sense to use this corresponding "hydrating" function to get your values in the right shape. Otherwise the caller still needs to do a bunch of custom preprocessing every time to "hydrate" the pubvalues, eg by calling a version of this fn that does what you describe, take an array of slugs and get their field info, and then having to "manually" match those hydrated slugs with the pubvalues.
But i agree that this all feels very annoying and not quite right. i feel like our "api" here is not good enough, and we should have a clearer way of "just doing what we want", namely validating that the pubvalues pass validation and exist in the community.
There was a problem hiding this comment.
I think the thing that throws me off is that consolidateFieldSlugs is a generic function that takes objects of any shape provided they each have a slug: string property.
Instead of updating an arbitrary object you could just return the results of the query, with a mapping from the object's identity (i.e. its index in the array, or its slug) e.g. instead of doing:
const hydrate = (arr: { x: number }[]) => {
return arr.map(v => ({ x: v.x, y: v.x + 1 }))
}
const final = hydrate(arr)you would do
const compute = (arr: { x: number }[]) => {
return arr.map(v => v.x + 1)
}
const final = compute(arr).map((y, i) => ({ x: arr[i].x, y }))Or compute could return a map from the input object to its result, like Map<{ slug: string }, TRes>.
You could argue the first approach is more succinct. But IMO the second is much more general purpose.
There's also a potential bug where the input objects have an existing fieldId or schemaName property which would be lost if it differs from what the function returns for each input object. I just feel it complicates the implementation for only the added benefit of consolidating some existing object with the results. Which the caller may or may not actually want to do.
There was a problem hiding this comment.
And sorry, to respond directly to your question, I don't think there's anything wrong with tacking on the slug property to the results returned by consolidateFieldSlugs if they would be needed downstream by the validation fn. I just don't see the need to attach all the additional properties from the objects originally provided to consolidateFieldSlugs.
…y signal intend to deprecate
| return { | ||
| id: field.id, | ||
| slug: field.slug, | ||
| ...valueMaybeWithRelatedPubId, |
There was a problem hiding this comment.
i also updated createPubRecursiveNew to use the new validatePubValues function
| const relatedPubs = await Promise.all( | ||
| Object.entries(body.relatedPubs).flatMap(async ([fieldSlug, relatedPubBodies]) => { | ||
| const field = pubFieldsForCommunity.find(({ slug }) => slug === fieldSlug); | ||
|
|
||
| if (!field) { | ||
| throw new NotFoundError(`No pub field found for slug '${fieldSlug}'`); | ||
| } | ||
|
|
||
| const relatedPubs = await Promise.all( | ||
| relatedPubBodies.flatMap(async ({ pub, value }) => { | ||
| const createdRelatedPub = await createPubRecursiveNew({ | ||
| communityId, | ||
| body: pub, | ||
| trx, | ||
| }); | ||
|
|
||
| if (!createdRelatedPub) { | ||
| throw new Error("Failed to create related pub"); | ||
| } | ||
|
|
||
| const pubValue = await autoRevalidate( | ||
| trx | ||
| .insertInto("pub_values") | ||
| .values({ | ||
| fieldId: field.id, | ||
| pubId: newPub.id, | ||
| value: JSON.stringify(value), | ||
| relatedPubId: createdRelatedPub.id, | ||
| }) | ||
| .returningAll() | ||
| ).executeTakeFirstOrThrow(); | ||
|
|
||
| return { | ||
| ...pubValue, | ||
| relatedPub: createdRelatedPub, | ||
| }; | ||
| }) | ||
| ); | ||
|
|
||
| return relatedPubs; | ||
| }) | ||
| ); | ||
| // this fn itself calls createPubRecursiveNew, be mindful of infinite loops | ||
| const relatedPubs = await upsertPubRelations({ | ||
| pubId: newPub.id, | ||
| relations: Object.entries(body.relatedPubs).flatMap(([fieldSlug, relatedPubBodies]) => | ||
| relatedPubBodies.map(({ pub, value }) => ({ | ||
| slug: fieldSlug, | ||
| value, | ||
| relatedPub: pub, | ||
| })) | ||
| ), | ||
| communityId, | ||
| trx, | ||
| }); |
There was a problem hiding this comment.
i made createPubRecursiveNew use the new upsertPubRelation function, slightly DRYer
3 participants
Wanted to get this in before i left on holiday.
No real time to write a proper PR, if the code is straightforward enough to understand/merge/continue working on it, please do! If not, I can fix all feedback once i'm back. See you in a week!
Issue(s) Resolved
#733
High-level Explanation of PR
Test Plan
Screenshots (if applicable)
Notes