Skip to content
This repository was archived by the owner on Oct 9, 2025. It is now read-only.

Conversation

@steve-chavez
Copy link
Member

@steve-chavez steve-chavez commented Apr 8, 2023


Add modifier: 'any' | 'all' option to the eq,like,ilike,gt,gte,lt,lte filters

This also makes me think about not, which doesn't have good DX. Maybe we can add another modifier to all operators like .eq('id', 1, {negate: true})

@soedirgo
Copy link
Member

This also makes me think about not, which doesn't have good DX. Maybe we can add another modifier to all operators like .eq('id', 1, {negate: true})

This doesn't cover the DX shortcoming in the general case - we'd still have issues with nested filters. I prefer if we properly solve this in v3, we can rework the filtering approach to be more of a fluent/builder pattern, e.g. Prisma.

@soedirgo
Copy link
Member

soedirgo commented Apr 14, 2023

I'm not sure about supporting any/all for eq, gt, gte, lt, lte: for eq there's in, for the rest, you could do a min/max on the array right?

Looks useful for like, ilike though - and I think textSearch as well. Though I'd prefer adding different methods for them since function overloading is a bit annoying for autocomplete, e.g. likeAnyOf, ilikeAnyOf

@steve-chavez
Copy link
Member Author

I'm not sure about supporting any/all for eq, gt, gte, lt, lte: for eq there's in, for the rest, you could do a min/max on the array right?

Yeah agree, so not needed there.

Looks useful for like, ilike though - and I think textSearch as well. Though I'd prefer adding different methods for them since function overloading is a bit annoying for autocomplete, e.g. likeAnyOf, ilikeAnyOf

Agree with likeAnyOf, ilikeAnyOf, would the all variants be useful as well? likeAllOf, ilikeAllOf.

any/all on textSearch doesn't work right now, but could be added later.

@steve-chavez steve-chavez marked this pull request as ready for review April 18, 2023 07:22
@soedirgo soedirgo merged commit ad74608 into supabase:master Apr 18, 2023
@github-actions
Copy link

🎉 This PR is included in version 1.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@steve-chavez
Copy link
Member Author

@soedirgo One advantage to using any over eq is that unlike in, you don't have to quote the values inside the array, in case they have special characters.

With that, maybe we could reconsider adding a eqAny.

@steve-chavez
Copy link
Member Author

This doesn't cover the DX shortcoming in the general case - we'd still have issues with nested filters.

@soedirgo Could you put an example of why it would fail with nested filters?

It should work as:

.eq('nested.id', 1, {negate: true})

right?

@soedirgo
Copy link
Member

soedirgo commented May 2, 2023

One advantage to using any over eq is that unlike in, you don't have to quote the values inside the array, in case they have special characters.

We'll need some form of escaping for both in and eq(any) - e.g. for eq(any) characters like {, }, , will break things.

Could you put an example of why it would fail with nested filters?

Sorry, I meant filters like and, or - you'd still have to use manual PostgREST syntax for those. Ideally things should work the same whether or not you're filtering as part of an and/or.

values: Row | Row[],
{
count,
defaultToNull = true,
Copy link
Member

Choose a reason for hiding this comment

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

@steve-chavez @soedirgo
Would this be a breaking change? The behavior prior to this was defaultToNull = false, where the missing fields get their default value specified on the table definition, right?

Copy link

@Vinzent03 Vinzent03 Jul 19, 2023

Choose a reason for hiding this comment

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

I don't think so.
From the postgREST doc:

Any missing columns in the payload will be inserted as null values. To use the DEFAULT column value instead, use the Prefer: missing=default header.

And the header is only set if defaultToNull is false here. So I think that's correct.

Copy link
Member Author

@steve-chavez steve-chavez Jul 19, 2023

Choose a reason for hiding this comment

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

@dshukertjr As Vinzent said, there's no breaking change.

defaultToNull was always true before. I would have liked it to make it the default behavior on PostgREST but it would have caused a breaking change (plus right now it also has a bit of perf loss).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see that this is only applicable when you are inserting multiple rows in bulk huh? I was thinking it applies to when you are inserting a single row as well. Maybe we could add a note about that in the comments

Copy link
Member

Choose a reason for hiding this comment

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

Actually it also applies when inserting a single row. But this only takes effect when specifying &columns=..., both for single & bulk inserts.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I think you're right, columns is only set when doing a bulk insert 😬 I'll add a comment about this and make the behavior consistent in v3. Thanks for the catch!

Choose a reason for hiding this comment

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

So there is currently a difference in inserting a single row as an object and a single row in an array with length 1?

Copy link
Member

Choose a reason for hiding this comment

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

It wasn't intentional, but yes

Choose a reason for hiding this comment

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

I just tried that and I think they are equal. I ran the following code in the test db of postgrest-js and other than the actual value of username they are equal:

let res1 = await postgrest.from('users').insert({ username: 'bot1' }).select()
let res2 = await postgrest
  .from('users')
  .insert([{ username: 'bot2' }])
  .select()
console.log(res1)
console.log(res2)

Especially is status = ONLINE, which is the default value of that column. So in both cases the default column is used and not null.

I ran further test understood it now. The missing fields are only mapped to null if a field is missing in one row, but present in another, because then that column is listed in &columns=. To use the default value in that case as well, you have to use the defaultToNull=false flag. For missing fields in a bulk insert with only one row, all fields are listed in &columns= and therefore the missing fields are mapped to the default.
So there is a difference in fields missing in all rows and fields missing in only a proper subset.
I just want to really understand that to properly document and implement it in postgrest-dart.

Copy link
Member

Choose a reason for hiding this comment

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

@Vinzent03 I think that's correct - so there isn't a difference between single row insert vs. bulk insert with 1 row after all, sorry for the back and forth.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error 'All object keys must match'

4 participants