Clarify that prev_content is part of unsigned, always

[jplatte]

Fixes #877.

Signed-off-by: Jonas Platte [email protected]

[jplatte]

I can't see CircleCI's logs without creating an account there?!

[turt2live]

INFO:matrix_templates.units:Reading event schema: /root/project/event-schemas/schema/core-event-schema/sync_state_event.yaml
Traceback (most recent call last):
  File "/root/project/scripts/templating/matrix_templates/units.py", line 262, in get_json_schema_object_fields
    res = process_data_type(props[key_name], required)
  File "/root/project/scripts/templating/matrix_templates/units.py", line 301, in process_data_type
    assert prop_type
AssertionError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/root/project/scripts/templating/build.py", line 285, in <module>
    substitutions=substitutions, verbose=args.verbose
  File "/root/project/scripts/templating/build.py", line 162, in main
    substitutions=substitutions,
  File "/root/project/scripts/templating/batesian/units.py", line 56, in get_units
    unit_dict[unit_key] = func()
  File "/root/project/scripts/templating/matrix_templates/units.py", line 780, in load_common_event_fields
    enforce_title=True,
  File "/root/project/scripts/templating/matrix_templates/units.py", line 310, in process_data_type
    enforce_title=enforce_title,
  File "/root/project/scripts/templating/matrix_templates/units.py", line 278, in get_json_schema_object_fields
    raise e2.with_traceback(sys.exc_info()[2])
  File "/root/project/scripts/templating/matrix_templates/units.py", line 262, in get_json_schema_object_fields
    res = process_data_type(props[key_name], required)
  File "/root/project/scripts/templating/matrix_templates/units.py", line 301, in process_data_type
    assert prop_type
Exception: Error reading property State Event.unsigned: 
Traceback (most recent call last):
  File "./scripts/gendoc.py", line 561, in <module>
    exit (main(args.target or ["all"], args.dest, args.nodelete, substitutions))
  File "./scripts/gendoc.py", line 446, in main
    run_through_template(templated_files.values(), VERBOSE, substitutions)
  File "./scripts/gendoc.py", line 312, in run_through_template
    subprocess.check_call(args)
  File "/usr/lib/python3.5/subprocess.py", line 271, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['python', '/root/project/scripts/templating/build.py', '-o', '/root/project/scripts/tmp', '-i', 'matrix_templates', '--substitution=%APPSERVICE_RELEASE_LABEL%=unstable', '--substitution=%PUSH_GATEWAY_RELEASE_LABEL%=unstable', '--substitution=%SERVER_RELEASE_LABEL%=unstable', '--substitution=%CLIENT_MAJOR_VERSION%=r0', '--substitution=%CLIENT_RELEASE_LABEL%=unstable', '--substitution=%IDENTITY_RELEASE_LABEL%=unstable', '/root/project/scripts/tmp/templated_identity_service.rst', '/root/project/scripts/tmp/[email protected]', '/root/project/scripts/tmp/templated_index.rst', '/root/project/scripts/tmp/templated_proposals.rst', '/root/project/scripts/tmp/templated_appendices.rst', '/root/project/scripts/tmp/templated_push_gateway.rst', '/root/project/scripts/tmp/[email protected]', '/root/project/scripts/tmp/templated_client_server.rst', '/root/project/scripts/tmp/templated_application_service.rst', '/root/project/scripts/tmp/[email protected]', '/root/project/scripts/tmp/templated_server_server.rst', '/root/project/scripts/tmp/[email protected]', '/root/project/scripts/tmp/[email protected]', '/root/project/scripts/tmp/[email protected]']' returned non-zero exit status 1

Enjoy! 😄

[jplatte]

Ah, I think I know what that's about. I was assuming just adding

unsigned:
  properties:
    # ...

when a referenced object in allOf already had the rest of the unsigned field's info would work, but I guess not.

[jplatte]

I guess I'll just have to figure out virtualenv or install everything I need for the scripts with my system package manager...

[jplatte]

After installing all the Python deps, it fails, but in a way unrelated to my PR / branch 😕

[jplatte]

Ah, was running the first thing I saw in the CircleCI config when I should have looked further down where the actual jobs are defined.

[turt2live]

and for the hat trick: signoff: #2647 (comment)

[jplatte]

I was of the impression I wouldn't have to do it anymore, at least not in every PR or sth., since I saw it in none of the other PRs. Added to the PR description.

[richvdh]

Unfortunately, I think this is a backwards-incompatible change to the spec, which requires an MSC. Synapse deliberately adds a prev_content field for compatibility with the spec.

[turt2live]

@richvdh this is documenting what synapse does though?

[richvdh]

@richvdh this is documenting what synapse does though?

I was under the impression that this PR was removing the prev_content from the event body (and instead putting it in unsigned)? Synapse currently (sometimes) puts it in both places, but that doesn't mean that we can just remove it from the event body.

[turt2live]

it sounds like a synapse bug if it's in both places, particularly if it's only sometimes. I can't find any links (possibly because they link to internal conversations somewhere), but I recall having this discussion a while ago and it was revealed that having it at the top level was a event v1 format thing, not the r0 format we currently try to use.

Just feels a bit silly to require an MSC for something that Synapse has done all along more consistently, particularly when this part of the spec uses Synapse as the reference.

[jplatte]

In the #matrix-spec discussion, somebody mentioned the format where synapse copies it from unsigned to the event object root is used for appservices, but only because Synapse doesn't yet implement the latest appservice spec.

The copying is done here, and that is the only place synapse ever puts prev_content into the event content root. I find it pretty hard to untangle where this actually ends up being used though.

[richvdh]

In the #matrix-spec discussion, somebody mentioned the format where synapse copies it from unsigned to the event object root is used for appservices, but only because Synapse doesn't yet implement the latest appservice spec.

This sounds like crossed wires. Afaik the appservice spec just follows the C-S spec here.

The copying is done here, and that is the only place synapse ever puts prev_content into the event content root. I find it pretty hard to untangle where this actually ends up being used though.

yes, it's used in a lot of places. Most of the C-S api that returns events, in fact.

[turt2live]

This sounds like crossed wires. Afaik the appservice spec just follows the C-S spec here.

The appservice spec follows the client-server spec, but Synapse doesn't follow the appservice spec because pre-r0 the appservice spec was using v1 event formats and Synapse never changed.

[jplatte]

If removing prev_content from the event object root is not an option because it is backwards-incompatible, could I re-add the stuff I removed here with a note that checking the one in unsigned is probably a better idea (since that's where synapse always includes it)? I'm not very keen on writing an MSC just to be able to make Ruma both spec-compliant and Synapse-compatible.

[richvdh]

The appservice spec follows the client-server spec, but Synapse doesn't follow the appservice spec because pre-r0 the appservice spec was using v1 event formats and Synapse never changed.

I'm failing to follow this. https://matrix.org/docs/spec/client_server/r0.6.1 is pretty clear that prev_content belongs in the object root, and AFAIK synapse puts it there. What exactly are you saying that synapse is doing that does not follow the appservice spec?

If removing prev_content from the event object root is not an option because it is backwards-incompatible, could I re-add the stuff I removed here with a note that checking the one in unsigned is probably a better idea (since that's where synapse always includes it)?

would that not mean mandating that servers put entries in both the event body and in unsigned? (as indeed synapse does, I believe, but I'm still not sure it's a sensible thing to require .)

I'm not very keen on writing an MSC just to be able to make Ruma both spec-compliant and Synapse-compatible.

I'm clearly failing to grok where Synapse isn't following the spec, so please can you spell it out like I'm 5?

[jplatte]

I'm clearly failing to grok where Synapse isn't following the spec, so please can you spell it out like I'm 5?

I don't actually know, funnily enough. I just know that some people have experienced issues with Ruma not detecting prev_content because synapse only returned it in unsigned. CC @poljar and @dkasak who might know more.

[richvdh]

I don't actually know, funnily enough. I just know that some people have experienced issues with Ruma not detecting prev_content because synapse only returned it in unsigned. CC @poljar and @dkasak who might know more.

you might be thinking specifically of the /sync endpoint, where it may well be that synapse behaves differently; in that case I am minded to treat this as a bug in the spec which is specific to /sync, which we can correct.

Of course, the fact that synapse returns prev_content in both the body and in unsigned for other APIs will mean that there will be no shortage of clients which rely on its presence even though that is not in the spec, which brings us somewhat back to square one.

[turt2live]

fwiw, this is from the js-sdk:
https://github.com/matrix-org/matrix-js-sdk/blob/2d73564eba62f0840241f1ebf4991f9acdefa5df/src/models/event.js#L275-L283

The comment there implies that the spec was just never updated for the v2 format.

[richvdh]

The comment there implies that the spec was just never updated for the v2 format.

the only api which ever made it to "v2" was /sync (which is why it has a different format to everything else), and even that only made it as far as "v2_alpha" (/_matrix/client/v2_alpha/sync) . when r0 was cut, we took all the CS endpoints at that point - including v1 and the nascent v2, and just added r0 as aliases to the existing endpoints.

So no, the spec was never updated for the v2 format, but that's correct, because none of the endpoints changed behaviour in practice.

[dkasak]

you might be thinking specifically of the /sync endpoint,

Yes, the problem was specific to /sync as far as I'm aware. We made matrix-rust-sdk move prev_content from unsigned to the root of the object if it only encounters it under unsigned as a workaround.

So to summarize the current situation (and check whether I understood it right):

• the spec says that prev_content should be put in the event content root for both /sync and other endpoints
• for endpoints other than /sync, Synapse includes prev_content both in the object root and under unsigned (though I'm confused by the word "sometimes" above?)
• for /sync, Synapse includes prev_content under unsigned only, contrary to what the spec currently says
• this is a consequence of the fact that only /sync was moved to the v2 format while other endpoints were left at v1

@richvdh, is your suggestion above that the spec should be changed to say that /sync in particular should return it under unsigned, unlike the other endpoints?

I've asked previously what the reasoning was for moving prev_content to unsigned in the first place and was told this is because prev_content isn't something that's sent over the federation. Is the plan then to change the spec for the other endpoints as well eventually?

[richvdh]

So to summarize the current situation (and check whether I understood it right):

your understanding seems right to me.

(though I'm confused by the word "sometimes" above?)

I haven't done a complete audit; I think what you said is correct.

@richvdh, is your suggestion above that the spec should be changed to say that /sync in particular should return it under unsigned, unlike the other endpoints?

yes.

I've asked previously what the reasoning was for moving prev_content to unsigned in the first place and was told this is because prev_content isn't something that's sent over the federation. Is the plan then to change the spec for the other endpoints as well eventually?

yes, that is the intention; however, that would be a backwards-incompatible change to the spec, which is fiddly to do and we always seem to end up with a million more pressing concerns.

[jplatte]

@richvdh, is your suggestion above that the spec should be changed to say that /sync in particular should return it under unsigned, unlike the other endpoints?

yes.

Alright, then there's a clear way forward now at least :)

I'll close this PR, but will comment here again in case I start working on the suggested solution.