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.

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

Thirdparty Entity Lookup API #1353

Merged
merged 11 commits into from Jul 12, 2018

Conversation

Projects
None yet
3 participants
@anoadragon453
Member

anoadragon453 commented Jul 3, 2018

PR of approved #1203

This is what is currently implemented, but undocumented, for Synapse and AS's such as IRC and gitter.

Having these formally specced would help with AS development on Dendrite.

@@ -147,7 +154,14 @@ paths:
Optional error information can be included in the body of this response.
examples:
application/json: {
"errcode": "COM.EXAMPLE.MYAPPSERVICE_NOT_FOUND"

This comment has been minimized.

@anoadragon453

anoadragon453 Jul 3, 2018

Member

Were we keeping this so that each AS has separate error codes or..?

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

Can we reserve the changing of error codes for a different PR?

@turt2live

Thank you for taking the time to document this! From what I can tell on a first pass it's accurate and matches real life - which is a great thing :)

I have left a number of comments about indentation of examples, spelling out words, etc. At about the halfway point I felt bad for leaving so many comments and decided to stop pointing them out - having them all fixed would be great.

@@ -91,7 +92,13 @@ paths:
}
schema:
type: object
500:

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

Do we really need to document 500? (applies to all responses)

This comment has been minimized.

@anoadragon453

anoadragon453 Jul 5, 2018

Member

I don't think we need to document it per endpoint... But we should at least state it somewhere. While building Dendrite I don't handle 500's because I assume that as the documentation doesn't mention them, they'll never be sent.

It doesn't seem like the C-S API mentions them either. Is it simply implied?

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

5xx errors are pretty much "The server exploded" and are generally treated as unhandled exceptions.

@@ -147,7 +154,14 @@ paths:
Optional error information can be included in the body of this response.
examples:
application/json: {
"errcode": "COM.EXAMPLE.MYAPPSERVICE_NOT_FOUND"

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

Can we reserve the changing of error codes for a different PR?

}
}
]
}

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

indentation on the example is off starting at line 252. It's a bad convention set by an auto-generator at some point, and we should stop following suit with it.

specific information about the various thirdparty networks that an AS
supports.
operationId: queryMetadata
parameters:

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

The query string needs documenting too.

This comment has been minimized.

@anoadragon453

anoadragon453 Jul 5, 2018

Member

Which query string are you referring to?

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

This particular route appears to have a query string associated with it, unless I'm blind and/or dumb.

This comment has been minimized.

@anoadragon453

anoadragon453 Jul 5, 2018

Member

You mean "protocol"? That's a path parameter, no?

examples:
application/json: {
"errcode": "COM.EXAMPLE.MYAPPSERVICE_UNAUTHORIZED"
}

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

indentation

Requesting this endpoint with a valid protocol name results in a list
of successful mapping results in a JSON array. Each result contains
objects to represent the Matrix room or rooms that represent a portal
to this 3PN. Each has the Matrix room alias string, an identifier for

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

Spell out "third party network" please (and below)

Retreive an array of thirdparty users from a Matrix ID.
operationId: queryUserByID
paramters:
- in: path

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

It's not in the path, it's in the query

alias.
operationId: queryLocationByAlias
parameters:
- in: path

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

it's not in the path, it's in the query

@@ -11,6 +11,18 @@ Unreleased changes
(`#1110 <https://github.com/matrix-org/matrix-doc/pull/1110>`_).
- ``POST /delete_devices``
(`#1239 <https://github.com/matrix-org/matrix-doc/pull/1239>`_).
- ``GET /thirdparty/protocols``

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

These aren't client-server endpoints and don't belong in the client-server changelog. Because the appservice spec is unreleased, it doesn't have a changelog and therefore no entry in the changelog is needed (for now).

@@ -12,6 +12,7 @@ targets:
- { 1: modules.rst }
- { 2: feature_profiles.rst }
- { 2: "group:modules" } # reference a group of files
- { 1: thirdparty_lookup.rst }

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

This doesn't seem to exist in this PR?

description: The Matrix IDs found with the given parameters.
examples:
application/json: [{
"userid": "@_gitter_jim:matrix.org",

This comment has been minimized.

@anoadragon453

anoadragon453 Jul 5, 2018

Member

@turt2live This came from the doc, but I'm assuming we should change to user_id like the rest of the spec?

@anoadragon453

This comment has been minimized.

Member

anoadragon453 commented Jul 5, 2018

Left out a pretty big chunk of the PR on accident. I've added them in the latest commit, and attempted to apply the fixes that you mentioned in the other files.

anoadragon453 added some commits Jul 5, 2018

@turt2live

This comment has been minimized.

Member

turt2live commented Jul 5, 2018

It looks like the Travis build is complaining about some sort of enum mismatch: https://travis-ci.org/matrix-org/matrix-doc/builds/400400675#L747

@anoadragon453

This comment has been minimized.

Member

anoadragon453 commented Jul 5, 2018

Still need to figure out why Travis is complaining, likely due to me not knowing how to write a schema for User correctly. Is that the right format?

Also should a similar schema be written for other objects?

@turt2live

I think I answered your schema question somewhere in here. If not, please yell at me. I don't see anything obvious that would cause Travis CI to complain, but I suspect it'll be easier to see with all the $ref stuff.

}
schema:
type: object
401:

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

ftr these errors should normally be split out to their own PR given they are unrelated to 3PE, however I'm indifferent in this case.

name: protocol
type: string
description: |-
The name of the protocol.

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

I think this is actually the protocol ID?

Also for brevity you don't need to have |- in there and can make it inline, if you wanted.

}
}
]
}
schema:

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

This should probably have it's own schema reference in a new folder called "definitions" under "application-service". The best approach would be to create that folder, copy event.yaml or something from the client-server API, and edit as needed.

"errcode": "COM.EXAMPLE.MYAPPSERVICE_UNAUTHORIZED"
}
schema:
type: object

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

All the errors should use the following to reference the error schema from the client/server API:

schema:
  "$ref": "../client-server/definitions/error.yaml"

Ideally errors become standardized across all APIs, however this works for now (and reduces duplication).

required: true
x-example: irc
- in: query
name: field1, field2...

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

looks like you can do something like this (untested):

in: query
name: searchFields
schema:
  type: object
  additionalProperties:
    type: string  # Maybe object? is there an 'any' type?
style: form
- application/json
produces:
- application/json
paths:

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

above here you'll need this:

securityDefinitions:
  $ref: definitions/security.yaml
paths:
]
}
}
schema:

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

for all the client-server schemas, use the $ref trick to reference ../application-service/definitions/protocols.yaml (or whatever the name ends up being). I'd highly recommend the example trick for that too.

"errcode": "M_NOT_FOUND"
}
schema:
type: object

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

this definitely needs a $ref to definitions/error.yaml

@@ -1,6 +1,772 @@
Tables of Tracked Proposals
---------------------------
This file is autogenerated by a jenkins build process

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

not sure what happened here, but we should leave proposals out of this PR

This comment has been minimized.

@anoadragon453

anoadragon453 Jul 6, 2018

Member

Probably the result of testing the proposals.py script. Nice catch!

@@ -0,0 +1,26 @@
Application Services

This comment has been minimized.

@turt2live

turt2live Jul 5, 2018

Member

I'm not sure it makes sense to introduce application services in this way in the spec. Typically these kinds of things just get introduced as modules (which also means not doing the change in targets.yaml)

@anoadragon453

This comment has been minimized.

Member

anoadragon453 commented Jul 12, 2018

See what you think of the changes now. I still haven't figured out the field1, field2... thing. I think I have the hang of $refs, examples and schemas now though.

@anoadragon453 anoadragon453 force-pushed the anoa/as_thirdparty_lookup branch 3 times, most recently from fb04ae1 to 93baf79 Jul 12, 2018

@anoadragon453 anoadragon453 force-pushed the anoa/as_thirdparty_lookup branch from 93baf79 to 114bcf1 Jul 12, 2018

anoadragon453 added some commits Jul 12, 2018

@turt2live

The only other thing is the definitions should be under definitions rather than definitions/schema to be consistent with the c2s API.

@@ -42,6 +42,7 @@ Summary
`Server Side Search`_ Optional Optional Optional Optional Optional
`Server Administration`_ Optional Optional Optional Optional Optional
`Event Context`_ Optional Optional Optional Optional Optional
`Application Services`_ Optional Optional Optional Optional Optional

This comment has been minimized.

@turt2live

turt2live Jul 12, 2018

Member

to be honest, I think it'd be better to have the module just be third party networks rather than introduce the concept of appservices. In the end, the server could technically have it's own approach outside of appservices to do this, despite the API being designed well enough to proxy straight through to the appservice.

This comment has been minimized.

@anoadragon453

anoadragon453 Jul 12, 2018

Member

fair enough

@anoadragon453 anoadragon453 force-pushed the anoa/as_thirdparty_lookup branch from 4649af0 to 291a4df Jul 12, 2018

@anoadragon453

This comment has been minimized.

Member

anoadragon453 commented Jul 12, 2018

Fixed!

@turt2live

Thanks for putting up with my endless review :D

@anoadragon453

This comment has been minimized.

Member

anoadragon453 commented Jul 12, 2018

Thanks for giving it!

@turt2live turt2live merged commit 91c59e7 into master Jul 12, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@KitsuneRal

That one went a bit too fast. I have two comments; without having those addressed, I cannot use the files to generate the client code.

required: true
x-example: irc
- in: query
name: field1, field2...

This comment has been minimized.

@KitsuneRal

KitsuneRal Jul 13, 2018

Contributor

May I see a piece of OpenAPI specification that describes this syntax? Or are we extending the OpenAPI specification by this?

This comment has been minimized.

@anoadragon453

anoadragon453 Jul 13, 2018

Member

Yeah, we were having trouble with having the correct syntax to express arbitrary amount of arbitrary field names and values, thus the .... If you know of a better way to represent this please let us know!

This comment has been minimized.

@KitsuneRal

KitsuneRal Jul 13, 2018

Contributor

Yeah, the issue is that I don't find a good option either. When specified in the body, one can use additionalProperties (coincidentally also mentioned in my comment nearby). But it seems there's no proper way to provide an open list in query.
Taking a chapter from C/C++ and also from conventions used in GNU utils --help output, I'd probably prefer to have fields... for the name, with the ellipsis having a special meaning that it's actually a list of various fields rather than a single field. There's a problem with type in this case because fields, generally, can be integer or boolean as well. Seems like we cannot do anything about it within OpenAPI 2.

This comment has been minimized.

@anoadragon453

anoadragon453 Jul 13, 2018

Member

Ok, so do you want to change field1, field2... to fields... now?

This comment has been minimized.

@KitsuneRal

KitsuneRal Jul 13, 2018

Contributor

Yep, let's do it like that. I will patch my code generator to understand that syntax as well.

This comment has been minimized.

@anoadragon453

anoadragon453 Jul 13, 2018

Member

Cool, alright.

This comment has been minimized.

@anoadragon453

anoadragon453 Jul 13, 2018

Member

Is there somewhere we should be documenting this "extension" btw?

This comment has been minimized.

@KitsuneRal

KitsuneRal Jul 13, 2018

Contributor

I believe it's the first such case, so I don't know where to better document it.

This comment has been minimized.

@anoadragon453

anoadragon453 Jul 13, 2018

Member

I've been told a good place. Will update the PR in a moment.

This comment has been minimized.

@anoadragon453

anoadragon453 Jul 13, 2018

Member

Documentation added!

# limitations under the License.
type: object
description: Dictionary of supported third party protocols.
example: {

This comment has been minimized.

@KitsuneRal

KitsuneRal Jul 13, 2018

Contributor

If there is a stable set of fields (and from the example I assume there is), it should be documented. "Lead by example" is nice but formal parameter specification should be in place.

This comment has been minimized.

@anoadragon453

anoadragon453 Jul 13, 2018

Member

We already have a single protocol object defined and fields marked, so if this was a list it would be trivial. However, I'm not sure how to effectively do a $ref for members in a dictionary/object?

This comment has been minimized.

@KitsuneRal

KitsuneRal Jul 13, 2018

Contributor

If I understand correctly what you want to express, you might find additionalProperties matching your needs, with $ref specifying the property schema.

This comment has been minimized.

@anoadragon453

anoadragon453 Jul 13, 2018

Member

I'll give it a shot, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment