WIP - add raw-media-endpoints config option

[dan-amoroso]

#1462 - still needs a test suite and config validation. I would appreciate some pointers as to where it is best to do the latter.

[dan-amoroso]

@steve-chavez I was wondering if we should take an "any" approach, where certain mime types are added to all requests, or an "otherwise" approach instead, which would apply the mime types only if no other setting matches the target. What do you think is best?

[steve-chavez]

@Dansvidania Oh, sorry I'm a bit lost here. Could you provide a sample config with the any?

I'm thinking you mean something like:

raw-media-endpoints {
  any = "image/png"
}

But not sure. A config with the otherwise would also help me understand the idea.

[dan-amoroso]

The intent is to give the possibility to define a catch-all clause in the config so that a default mime type can be made a raw media type for all endpoints via a special "keyword" (here I refer to any or otherwise but they could be something else)

The difference between the two approaches would be the effect for endpoints that have a specific definition in raw-media-endpoints (in the examples, endpoint images has a specific definition of raw media types)

With this config requests would accept both any types, and the types for their specific endpoint:

raw-media-endpoints {
  any = "mime/foo"
  images = "mime/bar"
}

a request to /images would accept both mime/foo and mime/bar as raw media types.

instead, given this config

raw-media-endpoints {
  images = "mime/foo"
  otherwise = "mime/bar"
}

a request to /images would accept image/jpg only.

What I am asking is whether you think this functionality is needed, and also whether such defaults should work in additive fashion (with any) or in alternative fashion (with otherwise). Or maybe if both special keywords should be included?

I hope I did a better job of explaining what I mean.

[steve-chavez]

@Dansvidania Ah, I understand the any/otherwise proposal now. One problem I see is that they could make all the other responses fail in a browser, same as raw-media-types. Say I have an example html page like the one I mention here, plus another one that I'd have in /rpc/admin.html. I could do this with the config:

raw-media-endpoints {
  any = "text/html"
}

Calling the html endpoints would work but all the others would result in the same issue as #1462 (text/html requested but more than one column was selected).

It'd be better to ask the user to be specific and instead do:

raw-media-endpoints {
  /rpc/index.html = "text/html"
  /rpc/admin.html = "text/html"
}

In general, I think these endpoints(html or image) will be few. So I'd go for no any/otherwise for now. If there's a need for a more generic config we could add that later. What do you think?

[steve-chavez]

Hey @dansvidania. Good to have you back!

config validation

We have been doing that on Main.hs, like for the roleClaimKey, panicking on failure.

What kind of errors would you expect? An invalid media type?

[dan-amoroso]

Hey @Dansvidania. Good to have you back!

Thanks. I am looking forward to finally making some contributions.

What kind of errors would you expect? An invalid media type?

I was mostly thinking about a warning message to state that raw-media-types is deprecated and that if both -types and -endpoints are configured, -tyeps is ignored in favor of -endpoints

[steve-chavez]

I was mostly thinking about a warning message to state that raw-media-types is deprecated and that if both -types and -endpoints are configured, -tyeps is ignored in favor of -endpoints

@Dansvidania Good idea. That would make migration to the new version easier.

Also, I have an alternative idea for the implementation, I think it could offer better UX. It would not need raw-media-types/endpoints, but instead it'd rely on overriding the Content-Type in SQL.

Here's the main idea.

Before we couldn't override the response media type through Content-Type, but #1435 made it possible to do so. Here's a test for that:

postgrest/test/Feature/PgVersion96Spec.hs

Lines 116 to 122 in 24064f8

	context "Override provided headers by using GUC headers" $ do
	it "can override the Content-Type header" $ do
	request methodHead "/clients?id=eq.1" [] mempty
	`shouldRespondWith` ""
	{ matchStatus = 200
	, matchHeaders = ["Content-Type" <:> "application/geo+json"]
	}

postgrest/test/fixtures/schema.sql

Lines 1693 to 1695 in 24064f8

	elsif req_path similar to '/(clients|rpc/getallprojects)' then
	perform set_config('response.headers',
	'[{"Content-Type": "application/geo+json"}]', false);

---

Now, what's missing is the ability to Accept the media type. This means that our content negotiaton(responseContentTypeOrError) should be able to obtain an extra media type at runtime(after the SQL is executed).

And actually, the overridden header is already obtainable in headers(you could check that with a traceShowId).

What needs to be modified is the responseContentTypeOrError check. That one should happen after obtaining the overridden header.

The check logic would mostly be the same, but it should consider that header .

After that, if the request Accept media type does not pass responseContentTypeOrError, then an error should be thrown. This is similar to what we do with application/vnd.pgrst.object+json(CTSingularJSON):

postgrest/src/PostgREST/App.hs

Lines 158 to 160 in 24064f8

	if contentType == CTSingularJSON && queryTotal /= 1
	then errorResponseFor . singularityError $ queryTotal
	else responseLBS status headers rBody

[steve-chavez]

@Dansvidania Let me know what you think. If you'd like to try the approach above, starting with support for GET RPC would be the simpler path.

[dan-amoroso]

@steve-chavez If I understand correctly the alternative you propose is for users to define the Content-Type header for responses coming from specific tables or RPCs by hard-coding it in the custom_headers() function?

If that is the case, I am not able to see the advantage in doing it like that; it seems like it would be a bit more involved to configure (and harder to maintain?). Could you help me understand what gives better UX?

Anyway, I added what I think is the implementation you suggested to the PR. Maybe we could offer both solutions at the same time?

[steve-chavez]

@Dansvidania I'm sorry. I wanted to say that for RPC it has better UX. Because you can edit the Content-Type in the function body. Based on the example mentioned here it'd be like:

create function images(id int) returns bytea as $$
begin
  perform set_config('response.headers', '[{"Content-Type": "image/jpeg"]', true);
  return select files.blob from files where files.id = file.id;
end $$ language plpgsql;

Then you'd be able to:

GET /rpc/images?id=1
Accept: image/jpeg

Content-Type: image/jpeg

<..jpeg..>

The function would accept and respond with the image/jpeg media type. Only by overriding the Content-Type.

[dan-amoroso]

I think I get it now, thanks. It does seem convenient to define the Content-Type right at the definition of the RPC.

I think this implementation could go side by side with raw-media-endpoints in case users want to use it for a table, but maybe offering a single way to handle binary output is simpler? I really do not know.

What do you think is best?

[steve-chavez]

Oh, I was just about to leave a comment about UX. I'll still leave it here in case it brings more clarity.

If that is the case, I am not able to see the advantage in doing it like that; it seems like it would be a bit more involved to configure (and harder to maintain?). Could you help me understand what gives better UX?

Sure. Besides #1462, in #1422, the use case is overriding the media type through RPC, which didn't work. I think that shows that overriding the media type(and making pgrst Accept it) in the function is the most intuitive way.

As you noticed though, the example of pre-request with the custom_headers function is contrived. Not ideal to do it like that, but it would come for free if we modify the logic for the accepted media types.

I think this implementation could go side by side with raw-media-endpoints in case users want to use it for a table, but maybe offering a single way to handle binary output is simpler? I really do not know.

I believe most users(also myself) will be suited by RPC. I agree that using pre-request for tables is weird, but I think we shouldn't get too ahead on the feature, that could risk a deprecation/removal later on.

So it'd be better to have a single way to handle binary output for now. If more convenience is needed later, we could see about raw-media-endpoints.

Btw, I'll mostly be working on main/Main.hs for some time. So feel free to refactor App.hs in the way you think is best.

[dan-amoroso]

@steve-chavez I have had a chance today to look in more depth at how to implement your solution.

I found no way to know the overridden Content-Type(s) before invoking the RPC (gucHeaders are obvliously available only after getting a row), and I could figure no way to postpone the responseContentTypeOrError check only for RPC invocation.

It makes a lot of sense that such a check would happen before anything is executed against the DB: given that we know the Accept header and that the Content-Type of the endpoint is static information, I do not think we should execute a trip to the DB just to possibly find out that the Accept and Content-Type are not compatible and return an error.

I imagine I either misunderstood something, or this is probably not the way to go.

[steve-chavez]

I do not think we should execute a trip to the DB just to possibly find out that the Accept and Content-Type are not compatible and return an error

Oh, an additional trip to the DB wouldn't be needed. We're already getting the responseHeadersF in each one of the generated queries. See:

postgrest/src/PostgREST/Statements.hs

Lines 138 to 146 in 43d71e9

	WITH {sourceCTEName} AS ({callProcQuery})
	{countCTEF}
	SELECT
	{countResultF} AS total_result_set,
	pg_catalog.count(_postgrest_t) AS page_total,
	{bodyF} AS body,
	{responseHeadersF pgVer} AS response_headers,
	{responseStatusF pgVer} AS response_status
	FROM ({selectQuery}) _postgrest_t;|]

postgrest/src/PostgREST/Private/QueryFragment.hs

Lines 202 to 206 in 43d71e9

	responseHeadersF :: PgVersion -> SqlFragment
	responseHeadersF pgVer =
	if pgVer >= pgVersion96
	then currentSettingF "response.headers"
	else "null" :: Text

I believe the overriden Content-Type should already be in headers.

[dan-amoroso]

@steve-chavez sorry i did not mean an additional trip. I do see that there would be only a single SQL execution.

What I mean is that, in order to see those overridden headers we need to execute the SQL (at least from what I see here)

Only then we are able to compare compare Content-Type against the Accept headers of the request to determine if they are compatible. In some cases, the headers might not match, at which point we would have executed the SQL (potentially an expensive computation?) for nothing.

I believe the fact that responseContentTypeOrError checks the compatibility of Accept and Content-Type before executing any SQL statement to be a valuable feature, that this change would need to break. Moreover, the overridden Content-Type header is not dynamic, so there is no real need for this to happen.

At this point, I am wondering if i isn't just better to deprecate raw-media-types and just promote the use of the Nginx snippet reported here. It is simple, clear and it does not impact performance.

[dan-amoroso]

As an alternative, unless the intent was to allow dynamically defining the request Content-Type (for example on a per-row basis) the overridden headers could be made part of the DbStructure.

In this way, they would be accessible before the request-specific SQL is executed, and still users would be able to define them in the body of RPCs.

What I mean is the following, for example.

create function images(id int) returns bytea as $$
begin
  perform set_config('response.headers', '[{"Content-Type": "image/jpeg"]', true);
  return select files.blob from files where files.id = file.id;
end $$ language plpgsql;

we could search invocations of set_config, parse them and add them to the ProcDescription type.

so the ProcDescription could look like this

data ProcDescription = ProcDescription {
  pdSchema      :: Schema
, pdName        :: Text
, pdDescription :: Maybe Text
, pdArgs        :: [PgArg]
, pdReturnType  :: RetType
, pdVolatility  :: ProcVolatility
--  for example Map Text [(Text,Text)] -> [("response.headers", [("Content-Type", "image/jpg")]]
, pdOverriddenConfig :: OverriddenConfigMap 
} deriving (Show, Eq)

This way, the request.headers proc config is available statically, and whenever a RPC is called we can math the Accept header of the request with the possible Content-Types of the response, without having to execute anything beforehand.

What do you think?

[steve-chavez]

@Dansvidania Sorry for the late reply, I'll try to address your concerns.

In some cases, the headers might not match, at which point we would have executed the SQL(potentially an expensive computation?)

Doing a lookup hContentType headers for ensuring content negotiation is right shouldn't add that much overhead,
don't see how that would impact peformance significantly.

for nothing

The same reasoning could be applied for checking the headers override(#1435) or the status override(#1541). We do that logic for nothing in most of the cases(most requests don't override), but thanks to that we're able to offer more flexible HTTP logic.

Moreover, the overridden Content-Type header is not dynamic, so there is no real need for this to happen.
As an alternative, unless the intent was to allow dynamically defining the request Content-Type (for example on a per-row basis) the overridden headers could be made part of the DbStructure.

Yes, the Content-Type is not really dynamic, but the solution of scanning the body of the functions could heavily impact
the startup time of the schema cache. And that would not work for overriding the Content-Type for GET through pre-request.

Also that approach would be much more complex to implement than the lookup above.

At this point, I am wondering if i isn't just better to deprecate raw-media-types and just promote the use of the Nginx snippet
reported here.

All of the HTTP logic features we support can also be done with Nginx(or Caddy, etc). The point is making them more convenient to use them with just PostgREST.

In general though, I agree there's a cost(like letting go the content negotiation check before executing SQL) in adding these kind of features. If the change is too complex, then of course Nginx is the answer. But if it's simple implementation-wise, we can have them and support interesting use cases(like the html rendering, or serving imgs).

[steve-chavez]

There's an alternative approach in #1548.

[steve-chavez]

@Dansvidania Check #1548 (comment)! We can keep doing the static check for content negotiation.

[dan-amoroso]

@steve-chavez i am trying to catch up with the developments that happened across different issues regarding this change. Static checking seems preferable to me so that's definitely good news, but at a first glance I did not understand what is the strategy in #1548

[wolfgangwalther]

@Dansvidania I'm copying the relevant parts (adjusted with the correct variable name we agreed on a couple of comments below that part) to give you the gist of it for RPC:

I don't really like the whole "raw-media-endpoints config option" approach - I think it would be a lot better, if we could somehow pass the information about which accept headers a function might respond to in the function definition itself. The best I could come up with in this regard would be to do it like this:

CREATE FUNCTION my_rpc_with_custom_accept_header (...) RETURNS bytea AS $$
  [...]
$$ LANGUAGE SQL SET pgrst.accept = 'application/msgpack';

The SET pgrst.accept = part would actually set a local variable available during function execution - but wouldn't be used for that purpose at all. However it would be available in pg_proc.proconfig as well and could be used when creating the schema cache. This way we could handle the content negotiation before making any query. We could implement some cleverness as well to allow multiple values (SET pgrst.accept = 'application/msgpack, application/whatever') and wildcards (SET pgrst.accept = 'image/*').

So, I think this boils down to the following:

• querying pg_proc.proconfig in the schema cache queries, extracting the pgrst.accept variable if set for all RPCs returning bytea, and storing this for every rpc - this should default to pgrst.accept = 'application/octet-stream'
• for all other RPCs that don't return bytea this should default to pgrst.accept='application/json, application/vnd.pgrst.object+json, text/csv' (the SET on the function should be ignored here - or maybe, as an improvement, be limited to only some of those 3 values, as those can be processed by postgrest by default)
• when receiving a rpc call, go through all rpcs with that name (might be an array for overloaded functions) and find the best match for the request's Accept header by looping through that rpc array for the possible overloads and checking (1) if the Accept header and the pgrst.accept for this function are compatible (e.g. requesting image/* would match pgrst.accept = 'image/png' and (2) if the current match is more "specific" than a match before (e.g. request has Accept: 'image/*', first rpc found has pgrst.accept = '*/*', next one has pgrst.accept= 'image/*', then both would match, but the 2nd would be taken, because it matches more specifically)
• if pgrst.accept is not a wildcard but a full content-type, the response header Content-Type can be set to pgrst.accept already. For wildcards, the RPC would have to do it, depending on the actually returned type.
• if the return type is bytea don't do any post-processing - if the return type is anything else do the regular post-processing as currently done, according to the accept header (I think that last point is already implemented)

@steve-chavez Am I missing anything?

[steve-chavez]

@wolfgangwalther I think we could omit looking at the return type.

Right now our binary output and plain text output work by using the string_agg function - which precisely works for text and bytea. If a function returns some other type than those two and raw output is requested an error from pg will be shown. The error message is not that clear(string_agg mismatch), but that could be improved later.

for all other RPCs that don't return bytea this should default to pgrst.accept='application/json, application/vnd.pgrst.object+json, text/csv'

I think it'd be simpler for the codebase to treat pgrst.accept as extra media types to accept. That way we keep our previous logic from raw-media-types but instead of the config we get the media types from the pg_proc.proconfig.

when receiving a rpc call, go through all rpcs with that name (might be an array for overloaded functions) and find the best match for the request's Accept

The responseContentTypeOrError function right now doesn't do full content negotiation logic. For example:

curl -H "Accept: application/*"  localhost:3000/projects
{"message":"None of these Content-Types are available: application/*"}

It does accept */* though. We could also leave that for another enhancement. We can ask the user to be specific for now, e.g. specify pgrst.accept = 'image/png, image/jpeg'.

What do you think?

[wolfgangwalther]

Right now our binary output and plain text output work by using the string_agg function - which precisely works for text and bytea. If a function returns some other type than those two and raw output is requested an error from pg will be shown. The error message is not that clear(string_agg mismatch), but that could be improved later.

I think the string_agg should still be used, because we still want to consolidate multiple bytea rows returned from the RPC into one row only for postgrest.

I would be completely fine with throwing that error now and improving it later. However, I guess the "requirement to look at the return type" was just a consequence of my thoughts in the next paragraph (see below), because applying this logic means that the decision whether to use the json_agg, string_agg or others (single object, csv) could not depend on the accept header anymore, so the return type would be most appropriate.

I think it'd be simpler for the codebase to treat pgrst.accept as extra media types to accept. That way we keep our previous logic from raw-media-types but instead of the config we get the media types from the pg_proc.proconfig.

I'm not sure whether the previous logic can be re-used. Previously this was not specific per function, right? So it was just allow or reject - and then see what happens.
I would expect a function that has SET pgrst.accept='image/png' to throw an error when called with Accept: application/json. I'm not sure that's the case with the current logic - even when extended for making decisions by function?
Along those lines, I guess I don't like the "extra" in "extra media types".

Also - hypothetically - I might want to create a function that does return application/json, but is not piped through the json_agg (or whatever it is exactly) we have. If I was to create a function that returns bytea and set pgrst.accept = application/json - I could provide my own transformation to json for my specific use-case.

It does accept / though. We could also leave that for another enhancement. We can ask the user to be specific for now, e.g. specify pgrst.accept = 'image/png, image/jpeg'.

Implementing it in steps is great, when the basic stuff allows to build on for enhancements. So if we only implement the specific case right now, thats fine with me. Would still need to iterate over that array for overloaded functions, I guess - so it would be "first match wins" right now? Or last one - whatever is easier to implement.

[wolfgangwalther]

If it is the return type that is looked at, it should probably be bytea or text for that, and the default be application/octet-stream or text/plain, respectively, to keep backwards compatibility.

[steve-chavez]

I'm not sure whether the previous logic can be re-used. Previously this was not specific per function, right?

True, though the proc is already available as a parameter. Should be straightforward to include it:

postgrest/src/PostgREST/App.hs

Line 123 in 8e4687f

app dbStructure proc cols conf apiRequest =

would expect a function that has SET pgrst.accept='image/png' to throw an error when called with Accept: application/json.

It won't throw an error, a bytea type will get converted to text when wrapped with json_agg. To confirm, try select json_agg(img) from images; on the postgrest_test schema.

Also - hypothetically - I might want to create a function that does return application/json

Yeah, but I think we should clear the task at hand first :). And actually If you want to return a custom json from a function, the default Accept: application/json will work fine, with our wrapper query. So maybe overriding the json output is not needed. That's why I think we should go with the "extra media types" approach.

[wolfgangwalther]

It won't throw an error, a bytea type will get converted to text when wrapped with json_agg. To confirm, try select json_agg(img) from images; on the postgrest_test schema.

I know it does not throw an error right now. But that's strange. Returning that image bytea in json just doesn't make sense for me, so I think I would want it to throw in the future!
It's less confusing when implementing RPCs as well. If you overwrite the Content-Type header in the function, it will be set to e.g. image/png - while the response is still in json, if you don't get the accept header right. But you would never know that, because your client wouldn't show the response to you that way. That just doesn't match.

Short: A RPC that serves a custom Accept header, will always have to set the Content-Type header accordingly. Content-Type and response body always have to match. "Extra" doesn't make sense in the context of one endpoint - it has to be "instead".

[steve-chavez]

It's less confusing when implementing RPCs as well. If you overwrite the Content-Type header in the function, it will be set to e.g. image/png - while the response is still in json, if you don't get the accept header right.

The responseContentTypeOrError function should already handle that if we pass it the pgrst.accept media type. What it doesn't make sense now is to set response.headers for Content-Type as in my previous example.
It used to be that the Content-Type was not overridable. For now we could be discourage this on the docs(pgrst won't accept the media type anyway, unless pgrst.accept is used).

I know it does not throw an error right now. But that's strange. Returning that image bytea in json just doesn't make sense for me, so I think I would want it to throw in the future!

Yeah, that could be done. The extra media types approach from now doesn't preclude us from allowing to override the default response later. It's just the easiest implementation.

[steve-chavez]

Maybe I'm missing something though. @Dansvidania What do you think it would be the simplest implementation?

[wolfgangwalther]

The responseContentTypeOrError function should already handle that if we pass it the pgrst.accept media type. What it doesn't make sense now is to set response.headers for Content-Type as in my previous example.

Passing pgrst.accept and using that as Content-Type is a corner-case, though. Later on, when pgrst.accept='image/*' is a thing, this is not possible anymore. Actually whenever multiple types are set - so e.g. for pgrst.accept='image/png, image/jpeg' as well (not sure if that's supported in the first step?). Since the browser will not ask for Accept: image/png directly, but ask for Accept: image/*, we can't make a decision about this anymore. This has to be done inside the RPC function by setting the header manually.

It used to be that the Content-Type was not overridable. For now we could be discourage this on the docs(pgrst won't accept the media type anyway, unless pgrst.accept is used).

Even if we only allow a single type for pgrst.accept for now, it seems wrong to discourage setting Content-Type manually and expect Content-Type: <pgrst.accept> to be the default - if we can already see it coming, that this will be the other way around.
If we reduce scope of implementation right now, I would rather not set the Content-Type header at all and encourage the user to always set it in the RPC.

The more I think about it, the more I think the current implementation of allowing Content-Type to be set, while defaulting to deliver json independently, has to be considered a bug. We should just not allow changing the Content-Type header when json or csv aggregation is used. This should force the header. Doing this should allow the "extra" implementation you are seeking as the first step, as it renders my arguments above void.

It's just the easiest implementation.

I think that's the bottom line of our differing opinions - I was/am not too concerned about finding the easiest implementation. Mostly because I hope to be able to just provide ideas, without implementing myself, here. Haha :)

The easiest implementation, that would still be consistent, could be:

• extend the current "extra" implementation for RPC-specific SET pgrst.accept.

• still allow to request and deliver json, json-object and csv with the current implementation in all cases - but force the correct header in this case

• encourage setting the Content-Type header in those RPC functions manually, by clearly documenting that the header will still be forced in case of requested json, json-object or csv

• if we choose to set pgrst.accept automatically as Content-Type, if possible, clearly document that this is a corner-case and not the default

I think this should still allow all the many next steps I suggested above, while keeping backwards compatibility.

[steve-chavez]

Since the browser will not ask for Accept: image/png directly, but ask for Accept: image/*, we can't make a decision about this anymore. This has to be done inside the RPC function by setting the header manually.

@wolfgangwalther You're right. Setting the Content-Type should be separate from pgrst.accept. I've missed this before.

The more I think about it, the more I think the current implementation of allowing Content-Type to be set, while defaulting to deliver json independently, has to be considered a bug. We should just not allow changing the Content-Type header when json or csv aggregation is used. This should force the header.

If instead of the "extra" we go with the "override" approach you suggested before, this might not be a big issue. Setting pgrst.accept = 'image/png' would make PostgREST not accept an application/json or text/csv request. I've thought better about this and I agree with you in that a user would not want to wrap a raw bytea in our json query.

Users might still override Content-Type on a non pgrst.accept function, but that would be an error on their part. We couldn't do much if they decide to set a json payload as audio/mpeg. However, later on, we could make overriding Content-Type only possible on a pgrst.accept function.

I think that's the bottom line of our differing opinions - I was/am not too concerned about finding the easiest implementation. Mostly because I hope to be able to just provide ideas, without implementing myself, here. Haha :)

Thinking about the easiest implementation made me get things wrong. Your ideas were very elucidating, keep them coming :)

[steve-chavez]

@wolfgangwalther @Dansvidania I'm thinking of doing a release this week. Mostly because I want to focus next release on PostGIS features(want to merge #1564 but not have it in this release).

@Dansvidania Let me know if I can help you with this one. I would like to include it in this release.

[dan-amoroso]

Sorry I will only be able to work on this after the 17th.. I will try to get to it asap.

Your discussion is a bit hard to follow for me, so a couple of test cases to guide the implementation would really help me get going.

[steve-chavez]

@Dansvidania I did an experiment on this branch to make sure of the implementation. So far the single media type case is working good. It's better than the raw-media-types approach in that the media type is set despite the client not requesting it explicitly(when Accept: */* is sent). Before we would default to JSON with the bytea wrapped in text, which was not really useful. Check these tests.

What's missing is the multiple media type case, e.g. if the client requests image/*. In this case we cannot set the media type and we would have to resort to letting the user set it on the function body.

[dan-amoroso]

apologies for being unable to work on this so far.

I can see the functionality is being implemented in #1582 , I will close this PR.