New /publicRoom APIs #388

erikjohnston · 2016-09-29T09:59:28Z

No description provided.

erikjohnston · 2016-09-29T10:07:23Z

@dbkr Can you just sanity check that it matches your understanding?

richvdh

Some nit-pickery. I realise that partly I'm requesting changes to existing stuff, but (a) it's a good opportunity to fix it, and (b) you're c&ping the broken stuff and I'd prefer not to see it spread.

A bunch of the comments apply to the POST version as well as the GET version.

richvdh · 2016-09-29T12:46:10Z

api/client-server/list_public_rooms.yaml

+                    canonical_alias:
+                      type: string
+                      description: |-
+                        The canonical alias of the room, if any. May be null.


Null, but not absent? I thought we avoided nulls in the API. If it really can be null rather than absent, it needs adding to required and to the example.

Ditto name, topic, etc, and ditto in the POST API.

Changed to be optional rather than null.

richvdh · 2016-09-29T12:47:19Z

api/client-server/list_public_rooms.yaml

+                      type: string
+                      description: |-
+                        The name of the room, if any. May be null.
+                    num_joined_members:


is this required? Needs adding to required if so; otherwise could do with explictly saying it's optional.

richvdh · 2016-09-29T12:47:44Z

api/client-server/list_public_rooms.yaml

+                type: string
+                description: |-
+                  A pagination token for the response, if there are any more results.
+              total_room_count_estimate:


required? optional?

richvdh · 2016-09-29T12:49:09Z

api/client-server/list_public_rooms.yaml

+        400:
+          description: >
+            The request body is malformed or the room alias specified is already taken.
+    post:


feels like POST /_matrix/client/unstable/publicRooms should be for adding rooms to the list. I don't really have any better suggestions though :/.

richvdh · 2016-09-29T13:19:55Z

api/client-server/list_public_rooms.yaml

+                type: string
+                description: |-
+                  A pagination token for the response, if there are any more results.
+              prev_batch:


what's this for? next_batch I get, but not prev_batch. It sounds like there might be requirements on the server which aren't being made explicit here?

richvdh · 2016-09-29T13:22:42Z

api/client-server/list_public_rooms.yaml

+                description: |-
+                  Optional filtering of the returned rooms.
+                properties:
+                    generic_search_term:


required? optional?

I appended (Optional) to the description, since I can't explicitly say required: [] at the top level :/

richvdh · 2016-09-29T13:25:10Z

api/client-server/list_public_rooms.yaml

+              next_batch:
+                type: string
+                description: |-
+                  A pagination token for the response, if there are any more results.


let's be more explicit here: a client determines that they have reached the end of the list by the absence of next_batch ?

richvdh · 2016-09-29T13:46:08Z

Also: needs a changelog entry, please.

erikjohnston · 2016-09-29T16:18:52Z

@richvdh PTAL

richvdh

sorry, still fonxing this on the null fields situation

richvdh · 2016-09-30T11:08:31Z

api/client-server/list_public_rooms.yaml

-                  A pagination token for the response.
+                  A pagination token that allows fetching previous results. The
+                  absence of this token means there are no results before this
+                  batch, i.e. this is the first batch.


Ah; is the direction of pagination implied in the token, unlike other APIs where there is an explicit direction flag? might be worth emphasising that.

I've updated to mention this.

richvdh · 2016-09-30T11:08:52Z

changelogs/client_server.rst

@@ -35,6 +35,8 @@
  - Add top-level ``account_data`` key to the responses to ``GET /sync`` and
    ``GET /initialSync``
    (`#380 <https://github.com/matrix-org/matrix-doc/pull/380>`_).
+  - Add pagination and filter support to ``/publicRooms``


... and more details in the response?

Added more details

dbkr · 2016-10-03T10:26:09Z

api/client-server/list_public_rooms.yaml

+          name: since
+          type: string
+          description: |-
+            A pagination token from a previous request, allowing clients to


Maybe worth clarifying that this can be either a prev or next token?

dbkr · 2016-10-03T10:30:05Z

api/client-server/list_public_rooms.yaml

+                type: string
+                description: |-
+                  A pagination token from a previous request, allowing clients to
+                  get the next batch of rooms.


or previous

richvdh · 2016-10-03T12:13:21Z

api/client-server/list_public_rooms.yaml

+              }
+        400:
+          description: >
+            The request body is malformed or the room alias specified is already taken.


This doesn't sound right. There is no request body for a GET, and there is no specified room alias.

Similarly on the POST endpoint.

richvdh · 2016-10-03T12:17:33Z

api/client-server/list_public_rooms.yaml

+        400:
+          description: >
+            The request body is malformed or the room alias specified is already taken.
+    post:


GET endpoint is missing tags

richvdh · 2016-10-03T12:26:55Z

api/client-server/list_public_rooms.yaml

+                  A pagination token that allows fetching previous results. The
+                  absence of this token means there are no results before this
+                  batch, i.e. this is the first batch.
+                  The direction of pagination is specified solely by which token


I think this would make more sense on the since parameter than here.

Not done. Needs doing on the POST endpoint too.

richvdh · 2016-10-03T12:30:00Z

api/client-server/list_public_rooms.yaml

+                type: object
+                title: "Filter"
+                description: |-
+                  Optional filtering of the returned rooms.


I think "Filter to apply to the results" reads better. No need to emphasise that it is optional (particularly since you don't for since and limit).

…public_rooms

richvdh · 2016-10-11T14:06:07Z

api/client-server/list_public_rooms.yaml

+                type: number
+                description: |-
+                  Limit the number of results returned, ordered by number of
+                  memebers in the room. Defaults to no limit.


richvdh · 2016-10-11T14:06:42Z

api/client-server/list_public_rooms.yaml

+                type: string
+                description: |-
+                  A pagination token from a previous request, allowing clients to
+                  get the next batch of rooms.


or previous

richvdh · 2016-10-11T14:07:20Z

api/client-server/list_public_rooms.yaml

+          type: number
+          description: |-
+            Limit the number of results returned, ordered by number of
+            memebers in the room. Defaults to no limit.


richvdh · 2016-10-11T14:11:41Z

api/client-server/list_public_rooms.yaml

+          name: limit
+          type: number
+          description: |-
+            Limit the number of results returned, ordered by number of


Do we return the busiest or least busy rooms? I'm assuming the former, but be explicit please.

Joined members? Invited members? Banned/left members?

Are the results returned ordered by number of members, or does this just apply to the limit? If the former, this should go elsewhere. If the latter, you probably want something like "Limit for the number of results returned. If there are more results, the rooms with the {most|fewest} members will be returned. By default, there is no limit." (You probably want that anyway).

Ditto for the POST API.

* order by *joined* members * clarify pagination direction behaviour

erikjohnston added 2 commits September 29, 2016 10:35

Spec canonical alias key

d81d0db

Spec new query params for /publicRooms

877a6cf

erikjohnston force-pushed the erikj/public_rooms branch from be36729 to df66ebe Compare September 29, 2016 09:59

Spec POST /publicRooms

130ea85

erikjohnston force-pushed the erikj/public_rooms branch from df66ebe to 130ea85 Compare September 29, 2016 10:01

erikjohnston changed the title ~~Erikj/public rooms~~ New /publicRoom APIs Sep 29, 2016

erikjohnston assigned dbkr and richvdh Sep 29, 2016

Add title and update description

c05ee18

richvdh requested changes Sep 29, 2016

View reviewed changes

erikjohnston added 3 commits September 29, 2016 16:58

Expand on description of pagination tokens

0dc94d5

Add changelog entry

212a4b5

Mark required fields as required

e854757

erikjohnston force-pushed the erikj/public_rooms branch from e0f329f to e854757 Compare September 29, 2016 16:13

Explicitlty mention total_room_count_estimate

e8b21f3

richvdh requested changes Sep 30, 2016

View reviewed changes

richvdh assigned erikjohnston and unassigned richvdh Sep 30, 2016

erikjohnston added 4 commits September 30, 2016 14:18

Mark booleans as required. Remove mention that things may be null

06649b6

Update changelog

5e99f47

Fix typo. Change responses in both places

a66a744

Explicitly mention that directio of pagination is specified by the token

a5bb182

erikjohnston assigned richvdh and unassigned erikjohnston Sep 30, 2016

dbkr requested changes Oct 3, 2016

View reviewed changes

dbkr removed their assignment Oct 3, 2016

richvdh requested changes Oct 3, 2016

View reviewed changes

richvdh assigned erikjohnston and unassigned richvdh Oct 3, 2016

erikjohnston added 4 commits October 11, 2016 14:24

Better wording

9d0960b

Add tags for GET endpoint

de68c16

Remove spurious 400 result

71c1fc5

Move explanation of lack of direction param

cafa9a0

erikjohnston removed their assignment Oct 11, 2016

Merge branch 'master' of github.com:matrix-org/matrix-doc into erikj/…

a69d6c6

…public_rooms

erikjohnston assigned richvdh Oct 11, 2016

richvdh requested changes Oct 11, 2016

View reviewed changes

richvdh assigned erikjohnston and unassigned richvdh Oct 11, 2016

Mention order of returned results

d66792d

erikjohnston assigned richvdh and unassigned erikjohnston Nov 21, 2016

richvdh added 2 commits November 21, 2016 12:22

Merge branch 'master' into erikj/public_rooms

dca1d0c

Final public_rooms clarifications

c175039

* order by *joined* members * clarify pagination direction behaviour

richvdh approved these changes Nov 21, 2016

View reviewed changes

dbkr approved these changes Nov 21, 2016

View reviewed changes

richvdh merged commit 02a8715 into master Nov 21, 2016

richvdh mentioned this pull request Jan 11, 2017

Paginatable and searchable public room directory listing (SPEC-120) #432

Closed

richvdh mentioned this pull request Aug 31, 2017

Spec POST /publicRooms API #868

Closed

afranke deleted the erikj/public_rooms branch September 22, 2021 10:26

New /publicRoom APIs #388

New /publicRoom APIs #388

Conversation

erikjohnston commented Sep 29, 2016

erikjohnston commented Sep 29, 2016

richvdh left a comment

Choose a reason for hiding this comment

richvdh Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Sep 29, 2016

erikjohnston commented Sep 29, 2016

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

richvdh Sep 29, 2016 •

edited

Loading

richvdh Sep 29, 2016 •

edited

Loading

richvdh Oct 3, 2016 •

edited

Loading

richvdh Oct 11, 2016 •

edited

Loading