-
Notifications
You must be signed in to change notification settings - Fork 21
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
Start of Sprint 2 #49
Open
sras
wants to merge
69
commits into
master
Choose a base branch
from
feature/spock-opaleye
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 21 commits
Commits
Show all changes
69 commits
Select commit
Hold shift + click to select a range
830d2ec
Formatting
sras a43ee57
Insert records directily with out having to convert to PGW types
sras 45c7a2d
Use the records directly without converting to thier PG counterparts …
sras 19b697b
Fix issue when dealing with enum field with default value
sras f6a52f0
Add housekeeping fields to Tenant
sras 46ba90a
Add housekeeping fields to remaining models
sras e7fef1e
Update housekeeping columns without lens
sras bc4b2df
Fix postgresql/haskell conversion issue and start using Lens
sras 4e25c7c
Made role api to use create_item function that uses lenses to deal wi…
sras f11a655
Convert more api function to use lenses
sras bb234be
Fix json generation
sras 785c7d7
Implementing generic update function
sras f286b54
Generic update function type checks now
sras 4781ca8
Refactor
sras e16f7a0
Convert everything to use lens
sras df887ed
Split the generic insert/update function in a separate module
sras 8308879
Fixed issue
sras 74e8743
Refactor
sras a8c94fd
Make created fields optional and updated field required for writing
sras 74eff26
Removed unused AllowAmbiguousTypes extension
sras 290cc61
Reenable create functions
sras 945ef75
Merge OpaleyeTypes with OpaleyeDef
sras e770e4c
Conver names to camel case and drop id from update functions
sras 04fc93d
Make id fields read only and remove unused Default instances
sras 93b6037
Fix warnings
sras 3734c41
Stylize
sras 07304e8
Refactor to use listToMaybe functions
sras 1a298cb
Start of Audtable api implementation
sras dd2d971
Convert all api function to use AuditM
sras bd209c5
Add wrapper to opaleye functions that runs in AuditM
sras 1004ca1
Add functions that does raw db updates in AuditM
sras 4b32bdd
Rename AuditM to AppM
sras ea708d7
Added TH functions to create auditable lenses
sras 6f80c26
Generate typeclasses for the auditable wrapper using TH
sras f940de5
Clean up warnings and start conversion of audit log to json Value type
sras 18e2dfe
Change the audit log to use the json Value object
sras c6f7d14
Added type syns for AuditM wrapper
sras 03a33cc
Convert all the api function to run in AppM
sras 7b4ba29
Use Auditable models in api functions
sras 6ebf760
Insert log to db on update calls
sras 79d5114
Test case for audit logs
sras f02c93a
Fix build issues
sras a995b2d
Fix typo in TH function name
sras 4a38268
Adding comments
sras 14e2d8a
Ammende TH functions to take types that are not sysnonyms into account
sras 4ce8966
Added schema files
sras 4b3fca4
Added list tenants endpoints for load testing
sras 82d2d78
Send mail via sendgrid
sras 69f172f
Added email templates and linked mail sending from create tenant json…
sras aa38a20
Add inline attachment embedding using cid method
sras 95f65e2
Fix double quoting of links and add hardcoded key
sras 27cf3b3
Fix double quote issue with quasiquoter and add a hardcoded key to th…
sras 480119f
Send mail from another thread and code refactor
sras 741ff7d
Add user services
sras 87b9674
Review modifications
sras 81f6fab
Cleaned up stack.yaml
sras 0a8c997
Use the updated mime-email library
sras 29a8a80
Wrapped the app monad around ExceptT to enable handling of uncaught e…
sras 65c5ccc
Trying to add request body to the error logs
sras 52c102b
Use a helper function to fetch env info and log it in case of an exce…
sras 0ce8cbf
Add request info to the log
sras cd991a1
Added airbrake source
sras e788e65
cabale file update
sras 6ec820b
Wrap the auditable types
sras f56c9c5
Got the refactor to compile
sras caa39c0
Rename files
sras 117f8db
Moved code to a separate package AppCore
sras 54dcd2e
Added removeAuditableRow function to api base
sras 39dbab3
Hide stuff in AppCore module
sras File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
{-# LANGUAGE FlexibleContexts #-} | ||
{-# LANGUAGE FlexibleInstances #-} | ||
{-# LANGUAGE FunctionalDependencies #-} | ||
{-# LANGUAGE MultiParamTypeClasses #-} | ||
{-# LANGUAGE OverloadedStrings #-} | ||
{-# LANGUAGE TemplateHaskell #-} | ||
|
||
module ApiBase where | ||
|
||
import Control.Lens | ||
import qualified Data.Profunctor.Product.Default as D | ||
import Data.Time (UTCTime, getCurrentTime) | ||
import Database.PostgreSQL.Simple | ||
import DataTypes | ||
import Opaleye | ||
import OpaleyeDef | ||
import OpaleyeTypes | ||
import Prelude hiding (id) | ||
|
||
create_row ::( | ||
HasCreatedat columnsW (Maybe (Column PGTimestamptz)), | ||
HasUpdatedat columnsW (Column PGTimestamptz), | ||
D.Default Constant incoming columnsW, D.Default QueryRunner returned row) | ||
=> Connection -> Table columnsW returned -> incoming -> IO row | ||
create_row conn table item = do | ||
current_time <- fmap pgUTCTime getCurrentTime | ||
let itemPg = (constant item) & createdat .~ (Just current_time) & updatedat .~ (current_time) | ||
fmap head $ runInsertManyReturning conn table [itemPg] (\x -> x) | ||
|
||
update_row :: ( | ||
HasUpdatedat haskells UTCTime | ||
, D.Default Constant haskells columnsW | ||
, D.Default Constant item_id (Column PGInt4) | ||
, HasId haskells item_id | ||
, HasId columnsR (Column PGInt4) | ||
) | ||
=> Connection -> Table columnsW columnsR -> item_id -> haskells -> IO haskells | ||
update_row conn table it_id item = do | ||
current_time <- getCurrentTime | ||
let updated_item = (put_updated_timestamp current_time) item | ||
runUpdate conn table (\_ -> constant updated_item) match_func | ||
return updated_item | ||
where | ||
put_updated_timestamp :: (HasUpdatedat item (UTCTime)) => UTCTime -> item -> item | ||
put_updated_timestamp timestamp = updatedat .~ timestamp | ||
match_func :: (HasId cmR (Column PGInt4)) => (cmR -> Column PGBool) | ||
match_func item = (item ^. id) .== (constant it_id) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove
it_id
from the function signature. We're already assuming that we have a field calledid
in theitem
itself. What is the advantage of passing theid
separately?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing the id separately explicitly specify which record to update. The record contains the data that the row should be updated with. The id specify the row that should be updated. Since those are separate things, I think those must remain separate.
Imagine a case when someone have a record where they have made some updates to the fields, and want to save to a certain Id. But imagine that they missed to change the id field. When they make the call, the wrong record is going to get updated. There is nothing warning the user that they didn't specify the id of the row that should be updated, because that was implicitly assumed to be the id that came with the updated record.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoa... stop right there. You don't get to "pick" which record should be saved to which ID! The ID is part of the record and self-identifies its row.
Which is the reason why ID should be a readonly field in the first place. I have yet to come across a valid use-case where one is mutating the primary key (ID) of a record -- either in the DB itself, or a record/object which represents the row in the DB.
Do you have a valid use-case for doing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are using an ORM that provides an Identity map and in a language with mutable values, you can mutate the fields of the object and call the update method to sync itself with the database.
But in Haskell, owing to it's immutable nature, this pattern is not possible. So in haskell, there cannot be a unique instance that represent the data for an entity. For example, a User record cannot be made to uniquly represent a row in the user table. Because that would require that another user record that represent the same row does not exist at the same time.
In a ORM with an Identity map, if a = user.post and user2 = a.author, then user and user2 refer to the same object. But in haskell, user and user2 can be different records, even though they hold same values, including the ID.
This means, we cannot treat records as authoritative representations of the underlying domain objects, and hence cannot use patterns that depend on this assumptions IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you call the record-type that Opaleye returns, which has the ID inside of it? The ID is the database identity of the record, which is what we're bothered with.
In fact, if we want to be pedantic about it, I would NOT EXPORT the
id
setter lens at all. No one should be able to change theid
of a record which represents a row in the DB. It has no practical purpose, and will result in bad things.As soon as the DB moves out of the database and goes to any other "temporary holding area" (record in Haskell, data-structure in Redis, etc.) it is no longer an "authoritative representation" at all. So, I'm unsure of how this really elucidates the Haskell or non-Haskell way of doing things.
And what exactly are we trying to prevent here? Can you give a real-life example of the use-case you are envisioning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A database row can only have one state at a given time. If you want a in-memory representation of a database row, you have to make sure that there are no more than one instance of it at the same time in the scope. If you cannot gaurantee that, then you will have inconstant state in your app, when there are conflicting representations of the same database row, and upon saving to the db, you will lose data.
I am not sure preventing updates to id will make any difference. Because either way, you are not mutating in place. So there is really no difference between updating id, and updating all the fields while keeping the id same. Because they both are different instances.
There is no difference between the result of the following operations
Starting with two User records,
Updating the id field of u1
is equalent to updating the name and age of u2 to that of u1, while keeping the id same
In both cases u3 has same values and is a different instance from either u3 or u2.
It is still authoritative representation even after it leaves the DB. In fact, once it leaves the DB, the row in the DB cease to be the authoritative representation, because the data might be getting mutated as it passes through the app. The row in the DB won't be valid until the entity gets saved back to the database.
What we are trying to prevent is ending up with inconsistent data. I gave you an example earlier. You reply was
But that breaks down as soon as you have multiple instances of a record with the same ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, but I'm not following your argument here. If you really want to get behind this argument, then the data-structures should be modelled on the lines of what Persistent has; wherein the identity (primary key) is not part of the value-record (other fields of the row). However, in Opaleye the ID is part of the record itself. So, there is no point mixing the two paradigms of thought over here.
Inconsistency is written all over this function signature:
There are two possible places where
TenantId
can be taken from. Which one is right? Why are there two places in the first place?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant an actual piece of code that you would genuinely write this way: