Skip to content
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

Throw on empty replica list #13

Merged
merged 2 commits into from
Oct 9, 2023
Merged

Conversation

casey-chow
Copy link
Contributor

In my testing, one of the things I noticed is that an empty list of replicas will lead to an unexpected undefined value. While I believe it would make sense for an empty list could be problematic in other ways, in situations like integration testing it may not make sense to set up a full replica instance.

This PR adds a coalesce to ensure that even when no replicas are instantiated, a valid Prisma instance (the primary) is returned. This way, we don't heave to worry about checking for an empty list and setting up types to condition on whether it is a replica or primary instance throughout the codebase.

Copy link
Contributor

@SevInf SevInf left a comment

Choose a reason for hiding this comment

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

Thank you for bringing the issue to our attention and submitting PR.

I agree that we need to handle empty list better than just randomly failing with undefined error somewhere deep inside the extension, however I am not convinced silently skipping replication in this case is the right approach: empty list of replicas can be a mistake and if we just accept it that would obscure this mistake and make it harder to fix.

What would you think about just adding a validation rule and throwing readable exception telling that at least 1 replica is required? Those who want to use extension conditionally could explicitly do so:

const basePrisma = new PrismaClient()
export const prisma = replicas.length === 0 ? basePrisma : basePrisma.$extends(readReplicas({ url: replicas }))

@casey-chow
Copy link
Contributor Author

Sure, that should be fine. I'll go ahead and udpate.

@casey-chow
Copy link
Contributor Author

@SevInf I've pushed a fix, let me know if you have any further comments.

Copy link
Contributor

@SevInf SevInf left a comment

Choose a reason for hiding this comment

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

@casey-chow Sorry for the late response. Thank you for doing the changes, looks good now.

@SevInf SevInf merged commit e9c39c6 into prisma:main Oct 9, 2023
@casey-chow casey-chow deleted the empty-url-list branch October 9, 2023 16:35
@SevInf SevInf changed the title Support empty replica list Throw on empty replica list Oct 23, 2023
@SevInf
Copy link
Contributor

SevInf commented Oct 25, 2023

Released in 0.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants