fix: rpc method #156

phamhieu · 2021-02-22T02:16:19Z

What kind of change does this PR introduce?

Create PostgrestRpcBuilder
Move rpc method from PostgrestQueryBuilder to PostgrestRpcBuilder
Fixes PostgrestClient.rpc() options param
Improve fetch response + error parsing (fixes Call rpc void function returns json parsing error supabase-js#132)
Fixes stored procedure unit tests

What is the current behavior?

When rpc method insides PostgrestQueryBuilder. User can create this chain
let res = await postgrest.from('users').rpc({ name_param: 'supabot'})
It's wrong.
no way to set options param

What is the new behavior?

No breaking change. postgrest.rpc usage is the same after moving it to PostgrestRpcBuilder
Expose rpc options param

phamhieu · 2021-02-22T02:18:09Z

isVoid: set 'Prefer': 'return=minimal' to request header. This will prevent json parsing error when calling void function
~~head: set method = 'HEAD'~~

~~Can we use a head request to call void function?~~
Postgrest stored procedure rpc doesn't support HEAD request.

I'm trying to fix this issue supabase/supabase-js#132

kiwicopple · 2021-02-22T03:55:28Z

It looks like the function was previously working. I wonder if we added something that broke it?

add isVoid param

I'm guessing the library is returning an error because we are trying to do some sort of await res.json() on an empty result? Perhaps it's better if we we just detect if (res.json) {} else .., so that the user doesn't have to add isVoid every time?

phamhieu · 2021-02-22T08:34:45Z

It seems to be a postgrest error. For void function, postgrest doesn't return anything.

my test function

CREATE FUNCTION public.void_func() 
RETURNS void AS $$
$$ LANGUAGE SQL;

When i create a unit test. It returns properly for my PR and the current master branch.
The unit test uses postgrest/postgrest:v7.0.1

However, when I test with a code playground and a real supabase project. postgrest doesn't complete the request for a void function.
I wonder if supabase project is using a different version of postgrest compare to the unit test? @kiwicopple

phamhieu · 2021-02-22T09:33:18Z

Confirm, it's postgrest error on this version postgrest/postgrest:nightly-2021-01-25-18-38-c93e8f9

kiwicopple · 2021-02-22T09:43:55Z

Let's wait to see what @steve-chavez thinks. I believe we're using the nightly version in prod so that we can update the config without downtime

soedirgo · 2021-02-22T11:55:10Z

I see, looks like PostgREST 7.0.1 is returning null for void-returning functions:

$ curl -s 'http://localhost:3000/rpc/void_func' -H 'Content-Type: application/json' -H 'Accept: application/json' -d '{"param":"foo"}'
null

But it returns nothing on nightly.

steve-chavez · 2021-02-22T12:23:39Z

But it returns nothing on nightly.

Yes, this was a side-effect of fixing functions that return SETOF on: PostgREST/postgrest@0c25f12.

Didn't realize it would break JSON.parse, I'll file it as a bug.

isVoid: set 'Prefer': 'return=minimal' to request header. This will prevent json parsing error when calling void function
Postgrest stored procedure rpc doesn't support HEAD request.

Postgrest do supports HEAD on RPC, but the return=minimal seems like a good workaround for now.

steve-chavez · 2021-02-22T23:13:00Z

Another suggestion for fixing the issue:

if (body === '') return undefined
return JSON.parse(body)

From PostgREST/postgrest#1762 (comment).

phamhieu · 2021-02-23T01:32:35Z

i improve fetch response parsing method. It can handle empty result

soedirgo

Just a couple comments, otherwise lgtm 👍

soedirgo · 2021-02-23T02:03:28Z

src/lib/types.ts

+            if (text && text !== '') {
+              try {
+                data = JSON.parse(text)
+              } catch (_) {
+                error = { message: 'Failed to parse json response' }
+              }
+            }


Maybe we should just do nothing in catch? Not sure if it's a good idea to "forge" PostgREST errors. Otherwise you'll also need other fields in PostgrestError:

postgrest-js/src/lib/types.ts

Lines 8 to 13 in 9ecb985

interface PostgrestError {

message: string

details: string

hint: string

code: string

}

Because users always expect json response from PostgREST. So if the response.text has content and we can't parse it to json. An error message is reasonable.

Actually, the error message is for us. I don't think users can do anything with this error. It's either PostgREST bug or postgrest-js bug.

Agreed, either way the user gets an unexpected error and files a bug. I guess we can keep old behavior and add special case for text === ''? Loud and explicit error > silent error IMO.

let do less. I remove try/catch block.

soedirgo · 2021-02-23T02:04:37Z

src/lib/types.ts

+          try {
+            error = JSON.parse(text)
+          } catch (_) {
+            error = { message: text }
+          }


Ditto here.

I can revert this to the original error = await res.json().
Just want to be safe here.

reverted to error = await res.json()

kiwicopple · 2021-02-23T03:05:43Z

src/lib/types.ts

-            data = null
+          const isReturnMinimal = this.headers['Prefer']?.split(',').includes('return=minimal')
+          if (this.method !== 'HEAD' && !isReturnMinimal) {
+            const text = await res.text()


will res.text ever be undefined here?

yes, PostgREST/postgrest#1762 (comment)

kiwicopple

Nice one, thanks @phamhieu 👍

github-actions · 2021-02-24T00:55:33Z

🎉 This PR is included in version 0.24.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

phamhieu added 6 commits February 22, 2021 09:43

refactor: move rpc method to PostgrestRpcBuilder

1bb6ada

feat: support void rpc function

46f36d1

fix: rpc unit tests

d4d1aea

chore: remove rpc from PostgrestQueryBuilder

7d0fc0e

fix: return PostgrestRpcBuilder

a1b6b77

chore: add options param description

c8ca89a

phamhieu requested review from kiwicopple and soedirgo February 22, 2021 02:23

phamhieu added 3 commits February 22, 2021 10:38

fix: Stored Procedure rpc not support HEAD request

c8a1785

fix: store procedure unit tests

4db91a2

chore: remove unused param

3533e4f

chore: create stored procedure returns void unit test

faf06a1

phamhieu added 4 commits February 23, 2021 09:21

fix: fetch response and error parsing

07abed9

chore: clean up

1409268

refactor: format code

b1f276a

chore: add comment

f32e3a0

soedirgo suggested changes Feb 23, 2021

View reviewed changes

kiwicopple reviewed Feb 23, 2021

View reviewed changes

kiwicopple approved these changes Feb 23, 2021

View reviewed changes

chore: revert try/catch error

bea7a74

soedirgo approved these changes Feb 23, 2021

View reviewed changes

phamhieu merged commit 2bbc435 into master Feb 24, 2021

github-actions bot added the released label Feb 24, 2021

kiwicopple deleted the fix/rpc-method branch March 1, 2021 14:49

kiwicopple mentioned this pull request Mar 1, 2021

void functions can no longer be invoked via SupabaseClient.rpc() supabase/supabase#762

Closed

	interface PostgrestError {
	message: string
	details: string
	hint: string
	code: string
	}

Uh oh!

fix: rpc method #156

fix: rpc method #156

Uh oh!

Conversation

phamhieu commented Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

Uh oh!

phamhieu commented Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiwicopple commented Feb 22, 2021

Uh oh!

phamhieu commented Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phamhieu commented Feb 22, 2021

Uh oh!

kiwicopple commented Feb 22, 2021

Uh oh!

soedirgo commented Feb 22, 2021

Uh oh!

steve-chavez commented Feb 22, 2021

Uh oh!

steve-chavez commented Feb 22, 2021

Uh oh!

phamhieu commented Feb 23, 2021

Uh oh!

soedirgo left a comment

Choose a reason for hiding this comment

Uh oh!

soedirgo Feb 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phamhieu Feb 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soedirgo Feb 23, 2021

Choose a reason for hiding this comment

Uh oh!

phamhieu Feb 23, 2021

Choose a reason for hiding this comment

Uh oh!

soedirgo Feb 23, 2021

Choose a reason for hiding this comment

Uh oh!

phamhieu Feb 23, 2021

Choose a reason for hiding this comment

Uh oh!

phamhieu Feb 23, 2021

Choose a reason for hiding this comment

Uh oh!

kiwicopple Feb 23, 2021

Choose a reason for hiding this comment

Uh oh!

phamhieu Feb 23, 2021

Choose a reason for hiding this comment

Uh oh!

kiwicopple left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

phamhieu commented Feb 22, 2021 •

edited

Loading

phamhieu commented Feb 22, 2021 •

edited

Loading

phamhieu commented Feb 22, 2021 •

edited

Loading

soedirgo Feb 23, 2021 •

edited

Loading

phamhieu Feb 23, 2021 •

edited

Loading