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

Extend the capabilities of PostgREST #280 #295

Merged
merged 38 commits into from Oct 4, 2015

Conversation

Projects
None yet
4 participants
@ruslantalpa
Collaborator

ruslantalpa commented Sep 28, 2015

itsdone

All tests passing (but not 100% ready for release).
I created this pull request to start the conversation so others can take a look at it. This needs fresh pair of eyes.

What currently works (i think) and how it was changed compared to prev version

  • The db structure is read only once at startup
  • / and /table[OPTINS] use only cached info
  • PUT / PATCH also use cached db info
  • Foreign keys for views are also detected
  • As before you can do
?select=*, col1::text, col2->>jp, ...
  • The major change is you can now include info from related tables (parents, children, many to many)
?select=*, child1(*), parent1(id, name), child2(*, subchild(*))
  • To add "filters" for related tables you can do
?child.col=eq.1&child.subchild.col=eq.2

What needs to be done

  • There are no tests for requesting related items (i think someone else should write them)
  • When there is an error in parsing the request or generating the query (no relation) i simply output the text, this needs to be "cast" somehow to be handled by errResponse
App.hs (81)
Left e -> return $ responseLBS status200 [("Content-Type", "text/plain")] $ cs e
  • Probably the code needs a bit of cleanup (formatting, indentation, rename some functions like allcolumns ...)

ruslantalpa added some commits Sep 7, 2015

@@ -1,27 +1,28 @@
{-# LANGUAGE QuasiQuotes, ScopedTypeVariables, OverloadedStrings #-}
{-# LANGUAGE OverloadedStrings #-}

This comment has been minimized.

@diogob

diogob Sep 28, 2015

Contributor

Couldn't we remove this as they are in cabal's default-extensions ?

@@ -0,0 +1,183 @@
{-# LANGUAGE OverloadedStrings #-}

This comment has been minimized.

@diogob

diogob Sep 28, 2015

Contributor

OverloadedStrings could be removed as is part of default-extensions

@@ -0,0 +1,183 @@
{-# LANGUAGE OverloadedStrings #-}
module PostgREST.Functions

This comment has been minimized.

@diogob

diogob Sep 28, 2015

Contributor

Functions is a very generic name. Maybe something like DatabaseHelpers or QueryBuilder would be more appropriate

@diogob

This comment has been minimized.

Contributor

diogob commented Sep 28, 2015

Awesome work men!
I can help with tests for the related tables cases if you'd like that.

apiRequest = first formatParserError (parseGetRequest req)
>>= addRelations schema allRelations Nothing
>>= addJoinConditions schema allColumns
where formatParserError = pack.show

This comment has been minimized.

@begriffs

begriffs Sep 30, 2015

Member

Use the general cs function rather than pack since it can figure out what to do.

import Data.Ranged.Ranges (emptyRange)
import qualified Data.Set as S
import Data.String.Conversions (cs)
import Data.Text (Text, pack)

This comment has been minimized.

@begriffs

begriffs Sep 30, 2015

Member

The line above that imports cs removes the need to import pack here.

, configJwtSecret :: String
}
argParser :: Parser AppConfig
argParser = AppConfig
<$> strOption (long "db-name" <> short 'd' <> metavar "NAME" <> help "name of database")
<$> strOption (long "db-name" <> short 'd' <> metavar "NAME" <> value "skin_test" <> help "name of database")

This comment has been minimized.

@begriffs

begriffs Sep 30, 2015

Member

The example value skin_test might seem strange. 😀

<*> strOption (long "db-user" <> short 'U' <> metavar "ROLE" <> help "postgres authenticator role")
<*> strOption (long "db-pass" <> metavar "PASS" <> value "" <> help "password for authenticator role")
<*> strOption (long "db-user" <> short 'U' <> metavar "ROLE" <> value "skin_test" <> help "postgres authenticator role")
<*> strOption (long "db-pass" <> metavar "PASS" <> value "skin_pass" <> help "password for authenticator role")

This comment has been minimized.

@begriffs

begriffs Sep 30, 2015

Member

Same for skin_pass.

colsRes <- H.session pool $ H.tx txParam $ allcolumns allRelations
let allColumns = either (fail . show) id colsRes
pkRes <- H.session pool $ H.tx txParam allprimaryKeys

This comment has been minimized.

@begriffs

begriffs Sep 30, 2015

Member

How about getting tblsRes, relsRes, colsRes, and pkRes from within the same session and transaction? I think it would be lower overhead. How about this:

metadata <- H.session pool $ H.tx txParam $ do
  tabs <- alltables
  rels <- allrelations
  cols <- allcolumns rels
  keys <- allprimaryKeys
  return (tabs, rels, cols, keys)

dbstructure <- case metadata of
  Left e -> fail $ show e
  Right (tabs, rels, cols, keys) ->
    return $ DbStructure {
        tables=tabs
      , columns=cols
      , relations=rels
      , primaryKeys=keys
      }

Also the all-lowercase names from PgStructure (like alltables) don't match the camelCasing of the rest of the code.

addParentRelation rel@(Relation s t c ft fc _ _ _ _) rels = Relation s ft fc t c "parent" Nothing Nothing Nothing:rel:rels
alltables :: H.Tx P.Postgres s [Table]
alltables = do

This comment has been minimized.

@begriffs

begriffs Sep 30, 2015

Member

camelCase

return $ map tableFromRow rows
allrelations :: H.Tx P.Postgres s [Relation]
allrelations = do

This comment has been minimized.

@begriffs

begriffs Sep 30, 2015

Member

camelCase

link2Relation _ = Nothing
allcolumns :: [Relation] -> H.Tx P.Postgres s [Column]
allcolumns rels = do

This comment has been minimized.

@begriffs

begriffs Sep 30, 2015

Member

camelCase

lookupFn _ _ = False
relToFk (Relation{relFTable=t, relFColumn=c}) = ForeignKey t c
allprimaryKeys :: H.Tx P.Postgres s [PrimaryKey]

This comment has been minimized.

@begriffs

begriffs Sep 30, 2015

Member

camelCase

@begriffs

This comment has been minimized.

Member

begriffs commented Sep 30, 2015

Some of those SQL statements inside PgStructure are works of art. Nice work.

find (\ col -> colSchema col == s && colTable col == t && colName col == c ) allColumns
findTable :: [Table] -> Text -> Text -> Either Text Table
findTable allTables s t = note ("no such table: "<>t) $

This comment has been minimized.

@begriffs

begriffs Sep 30, 2015

Member

I don't see findColumn or findTable used in other parts of the code. Are they here for future convenience?

, relColumn :: Text
, relFTable :: Text
, relFColumn :: Text
, relType :: Text

This comment has been minimized.

@begriffs

begriffs Oct 1, 2015

Member

What if

relType :: RelationType

where

data RelationType = RelChild | RelParent | RelMany

It looks like it is functioning as an enum. If we get its possible values out of strings it would make better use of the type system.

<> ") AS " <> table
where (B.Stmt subquery _ _) = requestToQuery schema (Node q forst)
-- the following is just to remove the warning, maybe relType should not be String?

This comment has been minimized.

@begriffs

begriffs Oct 1, 2015

Member

Looks like we had the same thought. 😄

ruslantalpa added some commits Oct 1, 2015

@ruslantalpa

This comment has been minimized.

Collaborator

ruslantalpa commented Oct 1, 2015

All suggestions/comments implemented and the latest master merged

)
] (cs $ fromMaybe "[]" body)
else cqs

This comment has been minimized.

@PierreR

PierreR Oct 1, 2015

Contributor

Remove the extra space ?

This comment has been minimized.

@ruslantalpa

ruslantalpa Oct 1, 2015

Collaborator

I made some changes so i am not sure to what part of the code you are referring to, could you check that part again and see if the spaces are removed?

This comment has been minimized.

@PierreR

PierreR Oct 1, 2015

Contributor

It is here :

https://github.com/ruslantalpa/postgrest/blob/master/src/PostgREST/App.hs#L86

But well it is just a space so don't worry ;-)

ruslantalpa added some commits Oct 1, 2015

@begriffs

This comment has been minimized.

Member

begriffs commented Oct 1, 2015

The child relations bugfix took care of the issue I was seeing locally.

I'm playing with an example database of films, directors, and competitions. This new embedding feature is powerful and fun. I can ask for directors with their films, or even competitions with their films (which goes through a nominations join table). Your branch automatically detects the necessary relations and magically works!

For the error case we should put it into a JSON object that matches the format of the other errors. So rather than

no relation between director and competition

it could be

{
  "hint": "could not find foreign keys between these entities",
  "details": null,
  "code": null,
  "message": "no relation between director and competition"
}

Same for parse errors. Rather than

"failed to parse filter (sdfjsdfjsf)" (line 1, column 1):
unexpected "s"
expecting "not." or operator (eq, gt, ...)

it could be

{
  "hint": "expecting \"not.\" or operator (eq, gt, ...)",
  "details": "unexpected \"s\"",
  "code": null,
  "message": "failed to parse filter (sdfjsdfjsf), (line 1, column 1)"
}
import qualified Data.ByteString.Char8 as BS
import Control.Applicative
import Network.HTTP.Types.Header
import PostgREST.Types ()

This comment has been minimized.

@begriffs

begriffs Oct 1, 2015

Member

Is the types import needed?

This comment has been minimized.

@ruslantalpa

ruslantalpa Oct 1, 2015

Collaborator

seems so (rangeParse)

This comment has been minimized.

@begriffs

begriffs Oct 1, 2015

Member

Hm, I'm able to compile the project with the import removed.

ruslantalpa and others added some commits Oct 2, 2015

Merge pull request #1 from diogob/fix_composite_fk_children
Adds a failing spec for child relation (comments) using a composite FK

begriffs added a commit that referenced this pull request Oct 4, 2015

Merge pull request #295 from ruslantalpa/master
Extend the capabilities of PostgREST #280

@begriffs begriffs merged commit 0f1b313 into PostgREST:master Oct 4, 2015

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment