Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Don't require end on /messages #1265

Merged
merged 4 commits into from
Jun 30, 2022
Merged

Don't require end on /messages #1265

merged 4 commits into from
Jun 30, 2022

Conversation

S7evinK
Copy link
Contributor

@S7evinK S7evinK commented Jun 28, 2022

As per the spec, the end key is not required if there are no more events for the user.

If no further events are available (either because we have reached the start of the timeline, or because the user does not have permission to see any more events), this property is omitted from the response.

@S7evinK S7evinK requested a review from a team as a code owner June 28, 2022 10:40
@DMRobertson
Copy link
Contributor

I think the spec's wording is actually stricter here: I think it's saying that end must be omitted if there are no more messages to retrieve.

Should we be asserting that end is not present in the json body?

@S7evinK
Copy link
Contributor Author

S7evinK commented Jun 28, 2022

Makes sense, yea.
Just tested that locally, Synapse isn't yet happy with that, though.

@richvdh
Copy link
Member

richvdh commented Jun 28, 2022

I thought that recently got fixed in synapse.

@DMRobertson
Copy link
Contributor

I thought that recently got fixed in synapse.

matrix-org/synapse#12903

@S7evinK
Copy link
Contributor Author

S7evinK commented Jun 28, 2022

Hm, what happens if the returned chunk count is less than the requested limit? Shouldn't that also result in end being omitted?
Or would that be this?

If so, we probably can't assert that end is actually missing in these tests, without extending the tests.

richvdh
richvdh approved these changes Jun 29, 2022
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm otherwise

tests/10apidoc/34room-messages.pl Outdated

scalar @{ $body->{chunk} } > 0 or
die "Expected some messages but got none at all\n";
})->then( sub {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

tests/10apidoc/34room-messages.pl Outdated
Comment on lines 174 to 175

# With no params this does "forwards from END"; i.e. nothing useful
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this needs repeating here

Suggested change
# With no params this does "forwards from END"; i.e. nothing useful

tests/10apidoc/34room-messages.pl Outdated

scalar @{ $body->{chunk} } > 0 or
die "Expected some messages but got none at all\n";
})->then( sub {

# Do another call to /messages, this time we don't expect to receive a "end" key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Do another call to /messages, this time we don't expect to receive a "end" key
# Do another call to /messages. This time we don't expect to receive a "end" key

tests/10apidoc/34room-messages.pl Outdated
@@ -158,12 +158,33 @@ sub matrix_send_room_text_message
})->then( sub {
my ( $body ) = @_;

# We should still get events and a "end" key, check it is actually there
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# We should still get events and a "end" key, check it is actually there
# We should still get events and a "end" key: check they are actually there

tests/10apidoc/34room-messages.pl Outdated
@@ -197,10 +219,12 @@ sub matrix_send_room_text_message
my ( $body ) = @_;

log_if_fail "Body", $body;


# We should still get events and a "end" key, check it is actually there
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# We should still get events and a "end" key, check it is actually there
# We should still get events and a "end" key: check they are actually there

tests/10apidoc/34room-messages.pl Outdated
@@ -209,6 +233,25 @@ sub matrix_send_room_text_message
scalar @{ $body->{chunk} } > 0 or
die "Expected some messages but got none at all\n";

})->then( sub {
# Do another call to /messages, this time we don't expect to receive a "end" key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Do another call to /messages, this time we don't expect to receive a "end" key
# Do another call to /messages. This time we don't expect to receive a "end" key

tests/10apidoc/34room-messages.pl Outdated
Comment on lines 241 to 242

# With no params this does "forwards from END"; i.e. nothing useful
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# With no params this does "forwards from END"; i.e. nothing useful

@S7evinK S7evinK merged commit a94cd1d into develop Jun 30, 2022
@S7evinK S7evinK deleted the s7evink/spec-messages branch June 30, 2022 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richvdh richvdh richvdh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@S7evinK @DMRobertson @richvdh