Re: [Jmap] new JMAP server for prototyping

[Jamey Sharp <[e-mail]>]

Thanks for answering my questions! I've snipped parts where your answer 
fully resolved my confusion, which was most of them really…

On Wed, May 19, 2021 at 01:05:48PM +1000, Neil Jenkins wrote:
>   > - Should creating multiple objects allow circular references, or 
>   > must it be a DAG of new objects? The former seems hard to get 
>   > right if any of the creations fail.
>
>   The relevant bit of the spec here is:
>
>    The final state MUST be valid after the "Foo/set" is finished;
>    however, the server may have to transition through invalid
>    intermediate states (not exposed to the client) while processing the
>    individual create/update/destroy requests. [...]
>
>   I don't think we actually considered a data type that supported 
>   circular references, but the way the current spec is written I would 
>   say that if the circular reference is valid for that data type and 
>   the final state is valid, then yes you would be allowed to do that 
>   in a single /set. But I agree that's a bit ambiguous, and I think 
>   you could probably forbid it in the /set description for the 
>   particular data type. Do you have an example in mind?

I don't have an example; I was actually hoping you'd say the intent was 
to forbid it. Since I'm writing an implementation which is agnostic to 
the particular data type, I need to figure out if this is a case I 
should allow. I haven't tested this case, but I think my current 
implementation fails all the creates in a cycle with invalidProperties.

Looking at a different bit of the text:

    In the case of records with references to the same type, the server 
    MUST order the creates and updates within a single method call so 
    that creates happen before their creation ids are referenced by 
    another create/update/destroy in the same call.

I think this wording is a little strange because I don't think destroys 
can reference creation IDs at all, right?

But more importantly: I think a strict reading would prohibit circular 
references in creates, since there is no ordering satisfying this MUST 
in that case.

Even under that reading, it only says that a create happens before 
"another" create which references it, but doesn't say anything about a 
create referencing itself, so that's ambiguous too I think.

>   > - Does the client need to ensure that it never uses a creation ID 
>   > of "foo" if there's some random string whose value happens to be
>   > "#foo", or does /set need to know which properties have Id type?
>
>   When processing a /set you need to know which properties have Id 
>   type (well you need to know all the types really in order to be able 
>   to validate the data).

Okay, sure, that's true. It's just that I wanted to substitute creation 
Ids first, without considering property types, and only validate the 
data afterward.

It seems easy enough for a client to use random creation Ids which are 
sufficiently large that they never collide with each other or with 
user-provided data. So if it's intended that clients may use strings 
which match their own creation Ids, I might just decide this 
implementation doesn't interoperate with clients that do that, which 
seems okay in a prototyping setting.

>   > - If the client uses the same creation ID in two method calls, 
>   > despite the "SHOULD" cautioning against it, and the second create 
>   > fails, should the first one's ID continue to be used? Is "the most 
>   > recently created item with that id" the most recent attempt, or 
>   > the most recent success?
>
>   Most recent success was the intention, but looking at it again I can 
>   see how you could interpret it both ways.

On Wed, May 19, 2021 at 01:56:19PM +1000, Bron Gondwana wrote:
>   If you break it, you get to keep both pieces!

Hah, yeah. "Most recent success" means a client which violates this 
"SHOULD" can't predict which object it's going to be referencing, and 
the target could even be of a different type than expected. But after 
all, you did tell them they shouldn't do that…

I guess I feel like that SHOULD would be better as a MUST. Getting 
unexpected results, but only when an error occurs in a different 
operation, sounds like a surefire way to suffering and support calls.

On Wed, May 19, 2021 at 01:05:48PM +1000, Neil Jenkins wrote:
>   > - What value should the position attribute in a /query response 
>   > have if the requested position is past the end of the results?
>
>   The definition of "position" in the response is:
>
>       The zero-based index of the first result in the "ids" array within
>       the complete list of query results.
>
>   So I agree it's undefined if there are no results (but I also think 
>   it's unimportant, because there are no results to position!). I 
>   would just return the requested position I think. It would probably 
>   be worth making this well-defined in a future revision.

I agree it shouldn't matter, but I think it might be helpful to define 
it any of these equivalent ways:

- the position immediately after the last item in the list
- the length of the list
- the minimum value of position for which you'd get the same response

A client can use calculateTotal to request that information, or negative 
position to implicitly reference it. But if they've gotten into a state 
where they thought there'd be more results than there really are, maybe 
it'd be nice to just tell them?

>   > - If a client has a sparse array of query results and gets a 
>   > /queryChanges response indicating that it should delete an object 
>   > that it didn't have in the array, then I don't see how it can tell 
>   > whether to shift down any of the objects it does have: the deleted 
>   > object could have been anywhere. [...]
>
>   Close. In this situation you have to invalidate any results in your 
>   sparse query after the first gap, but you can still keep the ones 
>   before.

Ohhhh. That makes sense, thanks. The spec is wrong here though, I think:

    The result of this is that if the client has a cached sparse array of
    Foo ids corresponding to the results in the old state, then:

    fooIds = [ "id1", "id2", null, null, "id3", "id4", null, null, null ]

    If it *splices out* all ids in the removed array that it has in its
    cached results, then:

       removed = [ "id2", "id31", ... ];
       fooIds => [ "id1", null, null, "id3", "id4", null, null, null ]

    and *splices in* (one by one in order, starting with the lowest
    index) all of the ids in the added array:

   added = [{ id: "id5", index: 0, ... }];
   fooIds => [ "id5", "id1", null, null, "id3", "id4", null, null, null ]

Here, "id31" is shown as removed, which wasn't in fooIds, but "id3" and 
"id4" come after a gap and weren't removed.

>   [...] the spec was written with the idea that /queryChanges would be 
>   implemented based on knowing which records have changed from last 
>   time, not based on having the old query cached and comparing it with 
>   the new one. (This approach is hard to scale effectively.)

Hah, definitely. Fortunately, I specifically don't care about 
scalability.

There are some neat tricks to play with storing deltas, and updating 
recent queries during /set, which I think might make it actually scale. 
But I'm not bothering to explore them, at least for now.

>   I'm not quite sure what you mean by "to the same length", but yes if 
>   you have the old query cached you could optimise the case where the 
>   upToId is not in the new results a bit further. But again, the spec 
>   is written to allow implementers to calculate query changes without 
>   caching the old query state.

I meant that if the client has cached the first 30 items, then upToId 
refers to the 30th item they knew about; a delta which gets them back to 
30 cached items seems like a reasonable thing to want.

I guess my feedback on this point is that it's another area that seems a 
little more focused on particular implementations than necessary. Which 
doesn't mean it needs to change, just that it's still on the list of 
things I'm confused about.

I can't quite figure out what client UX the upToId feature supports. I 
can imagine trying to keep the ten threads starting at #23 visible on 
the screen, or keep the thread list centered around the thread which is 
currently being displayed, but there's no reason to expect this will let 
me do either of those things, right?

Maybe if I understood the client side story better this would make more 
sense to me.

>   Great! This was really good feedback.

I'm glad to hear it!

>   > Currently all my tests are property-based randomized testing. I'd 
>   > like to think my tests might be interesting for other server 
>   > implementors. If there are test suites I might be able to reuse, 
>   > I'd love to hear about them.
>
>   Getting a good test suite for the JMAP spec is something we would 
>   love to have (and know from past experience is important for 
>   interoperability). I don't think anyone has published an open 
>   reusable one yet.

There are a few tests which only rely on core JMAP features that I think 
would make a good starting point. In particular, I really like that 
result references can be fully tested using two (or more) Core/echo 
calls in the same request.

>   > I've yet to tackle the session state, blobs, or push, but those 
>   > don't seem like they'll have hard questions about semantics, just 
>   > some possibly tricky engineering choices. We'll see how that goes.
>
>   I think queryChanges is the trickiest bit to get your head around 
>   semantically so hopefully now that's done the rest is not too hard, 
>   but do keep us informed of your progress and any other spec issues 
>   you find!

Will do. Thank you!
Jamey