Introduced raw-media-types config option #1349
Conversation
src/PostgREST/Config.hs
Outdated
| @@ -168,6 +169,7 @@ readOptions = do | |||
| <*> (maybe (Right [JSPKey "role"]) parseRoleClaimKey <$> optValue "role-claim-key") | |||
| <*> (maybe ["public"] splitExtraSearchPath <$> optValue "db-extra-search-path") | |||
| <*> ((\x y -> QualifiedIdentifier x <$> y) <$> dbSchema <*> optString "root-spec") | |||
| <*> (fmap encodeUtf8 <$> C.required "raw-output-media-type" (C.list C.string)) | |||
There was a problem hiding this comment.
I made it a required config key to make the behavior as explicit as possible, but I am not sure whats the preferred approach
There was a problem hiding this comment.
It definitely should not be required. Empty list [] should be default.
Also, I think text/plain should remain included since we always have had support for raw output for a text type and users have been using application/octet-stream for getting this.
There was a problem hiding this comment.
Ok. Do you mean text/plain and application/octet-stream should be included by default even if the config variable is left empty?
There was a problem hiding this comment.
@Dansvidania Yes, correct.
There was a problem hiding this comment.
@steve-chavez ok sounds good, thanks.
| @@ -6,3 +6,4 @@ server-host = "127.0.0.1" | |||
| server-port = 49421 | |||
|
|
|||
| app.settings.external_api_secret = "0123456789abcdef" | |||
| raw-output-media-types=["image/jpeg","image/png"] | |||
There was a problem hiding this comment.
New feature should not affect other tests configs. One dedicated config should be added.
There was a problem hiding this comment.
You should probably have one new config with text/html added to the list. We already have a test for that type somewhere, and that one should pass.
test/SpecHelper.hs
Outdated
| @@ -81,6 +81,8 @@ _baseCfg = -- Connection Settings | |||
| [] | |||
| -- No root spec override | |||
| Nothing | |||
| -- Raw output media types | |||
| ["text/html"] | |||
There was a problem hiding this comment.
What we should do is to have another config(smth like raw-media-types.config) that includes this setting, rest of the tests should run with raw-media-types of [].
Then you should move this test https://github.com/PostgREST/postgrest/blob/master/test/Feature/RpcSpec.hs#L467-L482 to its own Spec and have this spec use the previous config.
There was a problem hiding this comment.
@steve-chavez yep, I am working on it at the moment. I am hoping to push this before end of day.
|
Really important is that you add a couple of tests for the I mentioned that chrome sends: In that case pgrst would pick the If you have other browsers default Accept headers it would be great to test them as well. |
introduced implicit raw output media type independent from config variable added test for behaviors resulting from settings raw-output-media-types
| spec :: SpecWith Application | ||
| spec = describe "When raw-output-media-types config variable is missing or left empty" $ do | ||
| let firefoxAcceptHdrs = acceptHdrs "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8" | ||
| chromeAcceptHdrs = acceptHdrs "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3" |
There was a problem hiding this comment.
Really nice! This is what I also had in mind.
One thing though, I think it'd be better to test an endpoint such as /items?id=lte.3 and make sure we get the JSON payload. Root / follows some different rules.
To be extra sure we could also test a GET on rpc, an example /rpc/get_projects_below?id=3.
There was a problem hiding this comment.
Ok. I'll be "afk" till the 21st but will try to fix them asap.
Thanks for the feedback :)
src/PostgREST/Config.hs
Outdated
| @@ -273,6 +281,9 @@ readOptions = do | |||
| |## stored proc that overrides the root "/" spec | |||
| |## it must be inside the db-schema | |||
| |# root-spec = "stored_proc_name" | |||
| | | |||
| |## content types to produce raw output | |||
| |#raw-output-media-type=["image/png","image/jpg"] | |||
There was a problem hiding this comment.
Sorry, a nitpick. The config should be plural, end in media-types since it accepts a list.
Also, I think the name I proposed is too long. Ultimately we could shorten it to just raw-media-types.
There was a problem hiding this comment.
@steve-chavez makes sense. Will fix asap.
fixed tests in RawOuputTypesSpec to check GETting a record and a RPC
dan-amoroso
changed the title
test/Feature/RawOutputTypesSpec.hs
Outdated
| import SpecHelper (acceptHdrs) | ||
|
|
||
| spec :: SpecWith Application | ||
| spec = describe "When raw-output-media-types config variable is missing or left empty" $ do |
There was a problem hiding this comment.
raw-output-media-types should be raw-media-types here.
test/Feature/HtmlRawOutputSpec.hs
Outdated
| import SpecHelper (acceptHdrs) | ||
|
|
||
| spec :: SpecWith Application | ||
| spec = describe "When raw-output-media-types is set to \"text/html\"" $ |
There was a problem hiding this comment.
Ditto. s/raw-output-media-types/raw-media-types
There was a problem hiding this comment.
@steve-chavez ah, sorry, missed those occurrencies. Fixed them now.
|
Just realized that postgrest/src/PostgREST/Types.hs Lines 26 to 27 in c77ac24 postgrest/src/PostgREST/Types.hs Line 40 in c77ac24 postgrest/src/PostgREST/Types.hs Line 53 in c77ac24 |
fixed code formatting on HtmlRawOutputSpec.hs removed unused function from SpecHelper.hs
|
@steve-chavez thanks again for the feedback. All points should be addressed. |
|
@Dansvidania Great work on this PR. I'm going to merge it now. |
* extracted rawOutputTypes to config variable raw-output-media-types * removed CTTextHtml from Types.hs
Should solve #1336 and #1077
It seems functional but needs tests, for now I have only been able to extract rawContentTypes to Config.hs from Types.hs
It was used as a global value in App.hs so I had to make minor changes to a few helper functions.
Posting the WIP here to get feedback on the code while I work on the tests suggested by @steve-chavez