Fork me on GitHub
#untangled
<
2016-08-18
>
jasonjckn00:08:05

So i'm having trasient issues with load-field-action when it's used twice back-to-back

jasonjckn00:08:05

if I load-field-action on field :x, and :y, :y wins because it comes later

jasonjckn00:08:09

and :x is given :untangled.client.impl.om-plumbing/not-found

jasonjckn00:08:29

loading field :x works fine. alone. loading :y works fine alone. when it's back to back, the one that comes last wins

jasonjckn00:08:23

here's my current backend code that has the bug

jasonjckn00:08:28

(defmethod api-read :entitlements
  [{:keys [parser query] :as env} key params]
  (parser env query))

(defmethod api-read :ent-profile
  [env key {:keys [sso-id]}]
  (auth/profile-by-id! sso-id))

(defmethod api-read :ent-details
  [env key {:keys [sso-id]}]
  (auth/entitlements-by-id! sso-id))

jasonjckn00:08:32

it's a recursive parser

jasonjckn00:08:34

here's LOG output

jasonjckn00:08:02

16-08-17 23:49:31 fabric INFO [admin.system:21] - Read Request: [ :entitlements [(:ent-details {:sso-id "54d2497fe3e8d00800893017"})]
 ] 
16-08-17 23:49:31 fabric INFO [admin.system:21] - Recursive Read Request: [ :ent-details  ] ({:sso-id "54d2497fe3e8d00800893017"})
16-08-17 23:49:45 fabric INFO [admin.system:21] - Read Request: [ :entitlements [(:ent-profile {:sso-id "54d2497fe3e8d00800893017"})]
 ] 
16-08-17 23:49:45 fabric INFO [admin.system:21] - Recursive Read Request: [ :ent-profile  ] ({:sso-id "54d2497fe3e8d00800893017"})

jasonjckn00:08:11

so you can see we get 2 read requests

jasonjckn00:08:29

in this case :ent-details came first, so it gets assigned :untangled.client.impl.om-plumbing/not-found in the app-state

jasonjckn00:08:18

#_ (do

     (transact! [(entitlements/load-profile {:sso-id "54d2497fe3e8d00800893017"})])
     (transact! [(entitlements/load-entitlements-details {:sso-id "54d2497fe3e8d00800893017"})])
     )
this reproduces the bug, even though the load-field-actions are in separate transactions

jasonjckn00:08:40

if however at the REPL, I evaluate the first transact!, and wait, then evaluate the second transact!

jasonjckn00:08:49

then the bug doesn't reproduce (both fields get loaded data)

jasonjckn00:08:00

finally if I change my api-read code to the following (always return both fields, even though only 1 is requested at any given time) , then the bug goes away

(defmethod api-read :entitlements
  [{:keys [parser query ast] :as env} key params]
  (let [sso-id (-> ast :children first :params :sso-id)]
    {:ent-profile (auth/profile-by-id! sso-id)
     :ent-details (auth/entitlements-by-id! sso-id)}))

tony.kay20:08:59

Yeah, I saw the issue. Not had time to look at it

tony.kay20:08:38

I've never tried it myself, but I did think it would work. On second thought, I'm considering what happens in the pipeline. When you add two things at the same time it tries to combine them into a single query to process all at once. That means that the queries will run in order, and since they are keyed to the same thing (the result is a map, and the key is the ident) that this bad thing will happen.

tony.kay20:08:35

@jasonjckn There are a few possible "right things" we could do. One is say you should write the query yourself (that's all load-field is really doing: using load-data, but writing the query for you). Another would be to scan the queue and "merge" (or at least warn) of dupe keys. The easiest is probably to add support in load-field to take a single keyword or a vector of keywords. The latter would just generate the proper query. Then it would be considered incorrect to call it twice at once.

tony.kay20:08:46

We could also remove the "merge" bit from the queue processing. That was intended to make things more efficient, so I'm not really wanting to do that. I mean, load-data can exhibit a similar problem and it sort of breaks composition. So in that sense it is a bit of a concern

jasonjckn20:08:46

So i do like accepting a vector of keywords in load-field

jasonjckn20:08:52

definitely want that regardless

tony.kay20:08:53

middle ground would just be to make the merge smarter. Leave the things that have dupe top-level keys as separate network requests. This restores composability

tony.kay20:08:20

two load-field would run two network requests separately (unless you told it two fields with one call)

jasonjckn20:08:24

Yah, it sounds like the merge logic is broken, there should be no real difference between transact! twice back-to-back and transact! wait 1 second transact! but this merge 'optimization' is making these cases behave widly differently, so i would make sense to fix the merge or remove it until it works

jasonjckn20:08:40

right, i like that "two load-field would run two network requests separately (unless you told it two fields with one call)"

tony.kay20:08:08

Well, the idea is that initial loads might ask for 10 different things, and you'd like them to go all at once...but now that we have :parallel it is less of a concern

jasonjckn20:08:08

i'm all for the optimized version, but until it works proper, may as well just have 2 network requests

tony.kay20:08:36

although parallel uses separate network requests, it just doesn't serialize them

tony.kay20:08:55

Yeah, I think the general fix is to the merge

jasonjckn20:08:32

any idea when we can get this fixed?

tony.kay20:08:50

I can take a look this afternoon I think

jasonjckn20:08:03

great, let me know if there's anything I can do to help

tony.kay20:08:09

well, you can fix it 😉

tony.kay20:08:30

adding support for vector to load-field is probably relatively easy

jasonjckn20:08:42

well, it may take me a while to get up to speed , yah the latter is more suitable for me

jasonjckn20:08:51

feel free to skip the latter

jasonjckn20:08:00

perhaps i'll do it one day / it's not as pressing

jasonjckn20:08:18

although it's probably just 1 line change

tony.kay20:08:02

actually load-field is not an easy fix

tony.kay20:08:12

would be kind of invasive

tony.kay21:08:19

@jasonjckn (or anyone wanting to get their hands dirty) I added TODO markers in u.c.impl.data-fetch/mark-loading function. If you do those, it should fix the load-field issue. It is basic stuff. Note that the items in the items-to-load sequence have data access helper functions (e.g. data-query returns the query for the item to load).

tony.kay21:08:29

this is on develop on untangled-client

tony.kay21:08:51

Volunteers let me know. I will work on it later if no one picks it up.

jasonjckn21:08:49

Nice, i will take a look

jasonjckn21:08:19

let's say you do the optimized version where if the Q contains 2 items with the same key, you merge the subquery, is it really that hard to get right? consider my parser

(defmethod api-read :entitlements
  [{:keys [parser query] :as env} key params]
  {:value (parser env query)})

(defmethod api-read :ent-profile
  [env key {:keys [sso-id]}]
  {:value (auth/profile-by-id! sso-id)})

(defmethod api-read :ent-details
  [env key {:keys [sso-id]}]
  {:value (auth/entitlements-by-id! sso-id)})
the problem is my backend parser api-read :entitlements is called twice with 2 different queries, even though the client merged them, such that my LOG output on the backend is
16-08-17 23:49:31 fabric INFO [admin.system:21] - Read Request: [ :entitlements [(:ent-details {:sso-id "54d2497fe3e8d00800893017"})]] 
16-08-17 23:49:31 fabric INFO [admin.system:21] - Recursive Read Request: [ :ent-details  ] ({:sso-id "54d2497fe3e8d00800893017"})
16-08-17 23:49:45 fabric INFO [admin.system:21] - Read Request: [ :entitlements [(:ent-profile {:sso-id "54d2497fe3e8d00800893017"})]] 
16-08-17 23:49:45 fabric INFO [admin.system:21] - Recursive Read Request: [ :ent-profile  ] ({:sso-id "54d2497fe3e8d00800893017"})
The backend got 2 read requests even though the client ‘merged’ them. If instead we just have a single read request into api-read :entitlements in the backend, this would also fix the issue. Then the LOG output would be
16-08-17 23:49:31 fabric INFO [admin.system:21] - Read Request: [ :entitlements [(:ent-details {:sso-id "54d2497fe3e8d00800893017"})
                                                                              (:ent-profile {:sso-id "54d2497fe3e8d00800893017”})]] 
16-08-17 23:49:31 fabric INFO [admin.system:21] - Recursive Read Request: [ :ent-details  ] ({:sso-id "54d2497fe3e8d00800893017"})
16-08-17 23:49:45 fabric INFO [admin.system:21] - Recursive Read Request: [ :ent-profile  ] ({:sso-id "54d2497fe3e8d00800893017”})
my understanding of the issue with the optimized version is the "missing keys / ::om-plumbing/not-found" code see's a focused query from merged items-to-load, but the backend sees separate read requests for each item to load

ethangracer23:08:51

@currentoor @therabidbanana did you guys roll your own charts or did you use a library? trying to figure out how best to do charts with untangled and am curious to hear your experience

therabidbanana23:08:38

It's not too bad @kenbier did much of the heavy lifting there