-
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
Product types and queries and user/tenant activation #47
base: master
Are you sure you want to change the base?
Conversation
@wz1000 has the ProductQuery infra been hooked up to any handler? If not, can you please hook it up, so the flow is easier to understand? |
Also, if I understand the overall gist of
into a list of type I'm not completely able to understand how you're using the |
While I will take a little more time to understand the mechanics of this idea, as usual, you seem to have solved this problem very elegantly. Is there any way to abstract this even further so any such API endpoint is much easier to write (basically, reduce the boilerplate)? |
And I assume, this is possible for any DB library. In the case of Opaleye one would parse the URL params to an Opaleye specific type? Btw, I'm unable to figure out what type that would be? https://hackage.haskell.org/package/opaleye-0.5.1.1/docs/Opaleye-Operators.html |
When we use the |
This is the format for The API would be called like this:
Each What the monoid essentially does is take two JSON objects, and combines them to give a JSON object with the fields of both the objects(if any fields overlap, the first object's fields are kept). The monoid instance for functions is defined if the result type forms a monoid. Thus, when a function that takes a product and returns a JSON object containing its name is mappended to another function that takes a product and returns a JSON object containing its description, the result is a function that takes a product and returns a JSON object containing both the name and the description. |
Wow! I had missed that completely. We definitely need both of these wired-up to Servant handlers to complete the story! |
@wz1000 please confirm if you're wiring this up to Servant handlers. |
@saurabhnanda Done |
@saurabhnanda As persistent does not support joins, the definition for |
A few basic questions:
|
At a conceptual level, the pattern/architecture that you seem to be going towards is the following: transform the HTTP request (incoming JSON, query parameters, incoming patch/diff, etc), to functions/data-structures that represent SQL operations as closely as possible. Are you specifically aiming for this, or things just happen to be lining-up neatly this way? |
Neither ProductView nor ProductComparator are getting used right now.
This is the way QueryParams behaves by default.
Yes, but by explicitly stating that it is a |
Only |
I'm sure if you think hard enough you'll be able to state type-safe updates in terms of a Persistent interface, as well :) To me, both these approaches have something in common (which is significantly different from the standard Rails way of doing things) but I'm unable to put a finger of what exactly that is. |
What would it do to the Servant API signatures, if you use ProductView completely? |
When I took my first stab at writing ProductFilter, I implemented it using a simple Haskell function
where Once I had this, realised to use it I would have to load all the products in the database and then |
Not much. The return type would just change from |
So, is this sprint complete?
Tenant creation code still has some
Product creation as well, or just fetch and filter products? |
Was thinking more about this comment. Can you elaborate how Also, do you want to add anything to this PR? |
First, on the issue of correctness, there is the property that the product of two monoids is also a monoid. The monoid instances for ProductView and ProductFilter flow directly as a result of this. There is only one canonical way to make a monoid out of the product of two monoids. On the other hand, there are infinite ways to write a foldl/foldr, and so infinitely many ways by which to get it wrong. Second, monoid composition is associative. That means that |
Thanks for the quick primer on Monoids. Any comments on:
|
@wz1000 update, please. |
As of the commit I just pushed, the domain API is pretty much complete, other than photos and product updates. |
Comments:
(contd) |
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.
- Need some commentary on
OperationT
andTransactionT
lift $ runExceptT $ do | ||
time <- liftIO getCurrentTime | ||
when (null piVariants) $ | ||
throwError EmptyVariantList |
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.
Possible to avoid this runtime error by using length-restricted lists, perhaps?
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 can make the JSON parsing fail instead of throwing the error in createProduct
time <- liftIO getCurrentTime | ||
when (null piVariants) $ | ||
throwError EmptyVariantList | ||
case piType of |
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.
Possible to avoid this runtime check by making illegal states unrepresentable in our domain?
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.
Like the above, all we can do is shift failure to the parsing of the JSON instead of product creation.
import GHC.Generics | ||
import Data.Aeson | ||
|
||
newtype Price = Price { intPrice :: Int } |
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.
This type should map to NUMERIC
in the DB, which has the ability to represent fractionals without loss of precision (basically, it's not a standard float).
let urlSlug' = fromMaybe (sluggify piName) | ||
piURLSlug | ||
urlSlug <- lift $ makeUnique urlSlug' | ||
let dbProd = DBProduct { _dBProductAdvertisedPrice = advertisedPrice |
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.
Possible to use some generic programming to get rid of this boilerplate in all create APIs in our domain?
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 can't think of anything, as long the representation of the input is distinct from the representation in the database.
variants <- lift $ selectList [DBVariantProductID ==. pid] [] | ||
return $ Product (Entity pid prod) variants | ||
|
||
dbGetProductList :: MonadIO m => ProductFilter -> OperationT (TransactionT m) [Product] |
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.
should be an easier way to write this function!
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 is wrong with this?
go s _ = return s | ||
s' <- lift $ foldM go s (roleCapabilities role) | ||
if null s' | ||
runOperation :: (MonadIO m) |
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.
need some documentation on what's going on here.
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.
runOperation
basically takes a permission protected operation, and a user, and returns the result of the operation only if the user has the required permissions to run the operation. Otherwise it throws an error describing which permissions are necessary.
It does so by first generating the set of permissions required to perform the operation. Then by iterating over the capabilities of the user, it removes the permissions that the user has. If any permissions are leftover, it returns an error. Otherwise, it returns the result of the operation.
This ensures that the server never has access to the results of db API call that the currently logged on user shouldn't be able to access.
I'm missing something in the updation infrastructure again. Why is the following erroring out? What exactly does one need to pass to an
|
Also, just thinking aloud, if we have the |
You forgot to wrap the function in a
Also, the lambda is pretty redundant in this case. You can simply write
|
Not really, since Since the compiler has no way to check if the permissions you've required for any given operation are actually the permissions that operation requires, this shouldn't be that big of a tradeoff. |
|
Persistent provides |
Thats just the simple matter of replacing |
We need runtime checks somewhere. We can just shift the responsibility to the JSON parser instead. |
It is possible, we just have to add a |
Regarding removal of runtime errors:
Possible to add the relevant
Can you do that for the sake of completeness and a better API surface? Also, can you check if
Possible to implement, and close? |
I was wondering if it is worthwhile to have a |
Yeah, but right now we have a flat incoming data structure(JSON) and a flat target DB representation. Haskell is simply translating between the two representations. No real computations are performed with those data structures in Haskell. I felt it was overkill to translate to an intermediate "type-safe" haskell representation, only to immediately write it out to a flat DB representation. |
Well, we are trying to write a real-life webapp without actually writing a full-fledged real-life webapp :) So, while it may seem that it's overkill -- discovering the min-viable architecture for a large webapp is the overall goal we're targeting. Unless you feel, that this would be overkill even for a full-specced out, large webapp. |
@wz1000 any thoughts on #47 (comment) ? |
While haskell provides excellent support for manipulating records(via libraries like lens), record creation is still a major pain. I've actually thought about this a lot, and there are three approaches I could think of
It should be possible to build a more lightweight, specialised system for our purposes that is also nice to use, but it would require a heavy use of type level machinery. |
When choosing a representation for your data, the main consideration is the kinds of operations you expect to perform on your data. If you are mainly performing SQL reads/writes, the optimal representation would be pretty close to the DB row. If we expect some haskell operations to be performed on products, the representation we use will evolve accordingly(As it will when I get around to implementing product updates, with permissions and all) In short, its hard to choose the optimal representation for some data without knowing in advance what kinds of manipulations you expect to perform with that data. |
So, you want to tackle this when you handle product updates? Or push this to next sprint? On the UI side, we're trying to share models with the server. So, even if we feel that the server itself doesn't require a complex domain model, I'm sure there are advantages from a UI perspective. |
What about a |
Again, haskell provides little to no machinery for incrementally building up records. To do this, some kind of extensible record library would have to be used. To write Updater, I relied on Haskell's excellent support for polymorphism and function composition. Lenses can get values or modify existing values. Keyword there is "existing". If the field doesn't already exist, a lens can't set it. |
|
Still, there is no way to tell if all the fields have been set to a non default value. And you don't want things like the product name or price to be a default value. |
So, here is a practical flow:
Broad ideas, I know. But is this something that cant be done ? Will help us get rid of incoming JSON types, where the only reason they exist is because some underlying columns need to be "protected" |
Digestive functors present an applicative interface. Applicatives are great for parsing context-free grammars, but our product JSON is described by a context-sensitive grammar as the presence or absence of the weight fields depends on the result of parsing the product type field. So, in order to parse this we need the full power of monadic parsers. |
Do you know of any monadic parsers? Does this mean that digestive functors On 12 Nov 2016 1:44 pm, "wz1000" [email protected] wrote:
|
Most parsers, including Aeson's Applicative parsing is preferred wherever possible because applicatives can be analysed statically, and also give rise to much more efficient parsers.
Yes, unless you spilt the type into two, one for Digital and one for Physical products. Note that this doesn't necessarily mean that you have to use two separate records, as in Haskell this can be achieved by indexing the type with a product type parameter
However, to parse this type you would have to try parsing both types
|
Goals:
Done:
How queries on products will work
Servant provides an API to accept
QueryParams
in the URLA handler for this API can be implemented like so:
This is possible because
ProductFilter
, likeProductView
andProductComparator
compose and thus form a monoid. Now the user can query like this:and
mconcat
will compose all the filters supplied to give you a new filter with which we can write the SQL query.The mechanism is the same for
ProductView
andProductComparator
. SeeProductQuery.hs
for more details on how this is handled.