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

[FEATURE]: Add support for LIMIT 0 #2011

Open
sillvva opened this issue Mar 14, 2024 · 5 comments · May be fixed by #2255
Open

[FEATURE]: Add support for LIMIT 0 #2011

sillvva opened this issue Mar 14, 2024 · 5 comments · May be fixed by #2255
Labels
enhancement New feature or request

Comments

@sillvva
Copy link

sillvva commented Mar 14, 2024

Describe what you want

I have a query that will conditionally exclude the relational data by setting the limit to 0. Drizzle treats this as omitting the limit, however in PostgreSQL this should return an empty data set.

return await q.characters.findFirst({
    with: {
        user: true,
        logs: {
            with: {
                dm: true,
                magic_items_gained: true,
                magic_items_lost: true,
                story_awards_gained: true,
                story_awards_lost: true
            },
            orderBy: (logs, { asc }) => asc(logs.date),
            limit: includeLogs ? undefined : 0
        }
    },
    where: (characters, { eq }) => eq(characters.id, characterId)
});

I believe the cause of this issue is here:

const limitSql = limit ? sql` limit ${limit}` : undefined;

It is checking that it's truthy and 0 is not.

I believe it could be resolved with this simple change. limit is number | undefined and undefined fails the test, but 0 does not.

const limitSql = !isNaN(limit) ? sql` limit ${limit}` : undefined;

Not sure if there are any other places where this change would need to be made, though.

@sillvva sillvva added the enhancement New feature or request label Mar 14, 2024
@sillvva
Copy link
Author

sillvva commented Mar 14, 2024

For reference, I tried this method:
https://orm.drizzle.team/docs/select#conditional-select

const characters = await q.characters
	.findFirst({
		with: {
			user: true,
			...(includeLogs
				? {
						logs: {
							with: {
								dm: true,
								magic_items_gained: true,
								magic_items_lost: true,
								story_awards_gained: true,
								story_awards_lost: true
							},
							orderBy: (logs, { asc }) => asc(logs.date)
						}
					}
				: {})
		},
		where: (characters, { eq }) => eq(characters.id, characterId)
	});

But it resulted in a messy return type.

if (!character) return null;
const test = character.logs[0].dm // Error: dm does not exist
// It should also be erroring, because logs being returned at all is not guaranteed.

This sort of fixes that, but requires type assertions and giving up type safety.

.then((c) => c && { ...c, logs: "logs" in c ? (c.logs as LogData[]) : ([] as LogData[]) })

@JohnAllenTech
Copy link

Silly question perhaps but couldnt you make the with optional? ie something like

 with: includeLogs ? {
        user: true,
        logs: {
            with: {
                dm: true,
                magic_items_gained: true,
                magic_items_lost: true,
                story_awards_gained: true,
                story_awards_lost: true
            },
            orderBy: (logs, { asc }) => asc(logs.date),
        } :  {
        user: true,
    },

Not sure will the type inference be smart enough here though

@sillvva
Copy link
Author

sillvva commented Mar 21, 2024

@JohnAllenTech I already tried that. See my second comment.

This is my current workaround, but it would be nice to just have limit 0 to make this simpler.

	const character = await (async () => {
		if (includeLogs) {
			return await q.characters.findFirst({
				with: {
					user: true,
					logs: {
						with: {
							dm: true,
							magicItemsGained: true,
							magicItemsLost: true,
							storyAwardsGained: true,
							storyAwardsLost: true
						},
						orderBy: (logs, { asc }) => asc(logs.date)
					}
				},
				where: (characters, { eq }) => eq(characters.id, characterId)
			});
		} else {
			return await q.characters
				.findFirst({
					with: {
						user: true
					},
					where: (characters, { eq }) => eq(characters.id, characterId)
				})
				.then((c) => c && { ...c, logs: [] });
		}
	})();

@JohnAllenTech
Copy link

Ah apologies I see that now. As a potential workaround have you tried a negative limit? It seems it would pass the drizzle check but Postgres may have patched it. It used to be the same as 0

@sillvva
Copy link
Author

sillvva commented Mar 21, 2024

It returns an error: ERROR: 2201W: LIMIT must not be negative

@sillvva sillvva linked a pull request May 4, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants