-
Notifications
You must be signed in to change notification settings - Fork 211
Parse callbacks from Open API and create subscriptions #297
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
Conversation
@@ -5,7 +5,7 @@ | |||
"outDir": "lib", | |||
"sourceMap": true, | |||
"skipLibCheck": true, | |||
"lib": ["es2017"], | |||
"lib": ["es2017", "esnext.asynciterable"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to describe type returned by SubscriptionIterator
}, | ||
"callbacks": { | ||
"DevicesEvent": { | ||
"/api/{$request.body#/userName}/devices/{$request.body#/method}/+": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the callback path as a runtime expression.
It is evaluated when the subscribe resolver is triggered, based on the PayloadDefinition.
console.log(`http://localhost:${HTTP_PORT}/graphql`) | ||
console.log(`ws://localhost:${HTTP_PORT}/subscriptions`) | ||
|
||
new SubscriptionServer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle subscriptions sent by GraphQL clients
schema, | ||
onConnect: (params, socket, ctx) => { | ||
// adding pubsub to subscribe context | ||
return { pubsub } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we can override the pubsub instance inside the resolver. It might be useful to adapt on the backend PubSub API.
}) | ||
|
||
const pubsub = new MQTTPubSub({ | ||
client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the simulated backend (in example_api5_server.js) has an MQTT broker, we create a MQTT client to communicate through the pubsub instance.
} | ||
|
||
operations.push(operation) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The operations array is probably useless, but i was not sure if we might reuse it as reference for each callback later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... I don't think we should push the operation
to the data.operations
or else we will add the operation as a Query
or Mutation
field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each operation contained in callbacks is counted as Subscription
because these operations are filtered later with isSubscription
.
That's why i thought it might be useless to create this new array, unless we process them differently, but it didn't seem necessary.
options: InternalOptions | ||
): Operation[] { | ||
const operations: Operation[] = [] | ||
for (let callbackName in callbacksObject) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retrieving Operations for each PathItemObject found in CallbackObject ...
@@ -985,7 +1103,8 @@ export function getSecurityRequirements( | |||
path: string, | |||
method: string, | |||
securitySchemes: { [key: string]: ProcessedSecurityScheme }, | |||
oas: Oas3 | |||
oas: Oas3, | |||
callback?: CallbackObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an optionnal CallbackObject to be able to reuse frunctions provided by oas_3_tools
for (let cbName in oas.paths[path][method].callbacks) { | ||
for (let cbPath in oas.paths[path][method].callbacks[cbName]) { | ||
numOps++ | ||
// resolve $ref ?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still unsure if we should count deeper, each path contained in a callback as distinct callback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure either. It depends on the use cases. Do we expect people to have multiple operation objects on the same callback object? I'm guessing no... If that is the case, then we not count deeper.
We should however ensure that an operation object exists. Let's just assume that there can only be one and throw a warning if multiple ones are found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes echo to the comment above on preprocessor.ts
Maybe counting one operation would be easier to start with and we could slowly iterate to count and handle properly all of them later.
operation, | ||
data | ||
}) | ||
|
||
if (operation.isSubscription) { | ||
const responseSchemaName = operation.responseDefinition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If operationType is Subscription, then create a specific field with subscribe and optionally resolve
@getlarge Wow!!! This is an incredible amount of work! We will need some time to review this but this has been an amazing surprise! |
I'm glad that it makes you react this way. |
// would be a channel to subscribe on the resolver | ||
// but if callback object contains several operations | ||
// how to be sure that the returned Graphql type would be the same ? | ||
// By "forcing' a common response schema for every operation within the CB ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We encountered a similar problem when creating Query
and Mutation
fields, which may have multiple successful response objects based on the HTTP code. In that case, we simply picked the first successful response object. Perhaps there are ways to improve this in the future.
The other solutions would be try to create a union (as we do when resolving oneOf
) or creating an object type with the superset of all fields (assuming that all response are objects as we do when resolving anyOf
) or defaulting onto a JSON type.
For now, I think we should just pick an arbitrary one and throw a warning.
Edit: I think I misunderstood the question. I think the Subscription
field names should be a combination of the path and the cbName. The should not overlap.
operation, | ||
data | ||
}) | ||
|
||
if (operation.isSubscription) { | ||
const responseSchemaName = operation.responseDefinition | ||
? operation.responseDefinition.graphQLInputObjectTypeName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should use graphQLObjectTypeName
instead of graphQLInputObjectTypeName
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it would make sense, probably a copy/paste mistake.
data: PreprocessingData | ||
): { [key: string]: CallbackObject } { | ||
const callbacks = {} | ||
const endpoint: OperationObject = oas.paths[path][method] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small nitpick, but I would call this an "operation" instead of "endpoint"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes probably, i reused the same terminology of above functions.
callbacksObject[callbackName] | ||
// Make sure we have CallbackObject: | ||
if (typeof (callbackObject as ReferenceObject).$ref === 'string') { | ||
callbackObject = resolveRef( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check for references here? If I recall correctly, the only time we check for references is dealing with schemas that may be contained in the components object (related link).
Although, now that I've done some digging, I see that there are many other references we do not check for in general. Might need to file an issue about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It allows to pseudo validate CallbacksObject
and also to recompose them since they could be $ref to describe the CallbackObject
and/or the PathItemObject
.
Then we can safely go deeper to parse the operation that each PathItemObject
contains in processOperationCallbacks
.
Maybe i got it wrong ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should all the callbacks to the operation
object. I'm not entirely sure we want to resolve the references here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since getEndpointCallbacks
is called before processOperationCallbacks
and that the last one needs information about the whole callback content to compose the callback operation that is saved in PreprocessingData
, i thought it was required to resolve all the potential references.
What would be the alternative then ?
@@ -155,6 +355,17 @@ export function preprocessOas( | |||
// Links | |||
const links = Oas3Tools.getEndpointLinks(path, method, oas, data) | |||
|
|||
// Callbacks containing [key: string]:PathItemObject | |||
const callbacks = Oas3Tools.getEndpointCallbacks( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if processOperationCallbacks
should return its own operation array (instead of appending it to data.operations
) and callbacks
here should be set to that array instead of being set to getEndpointCallbacks
, which essentially just returns the raw callback objects, if I understand correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, here callbacks
are still raw.
I was also hesitating, but it seems safe to process them as operation.
After all, callbacks are composed of OperationObject
, so why not ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going back to this thought, I think callbacks
should just be another field in PreprocessingData
and we should populate it in preprocessOas
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good possibility, then i think we would not need the isSubscription
flag since subscriptions would be composed from data.callbacks instead of data.operations ( in translateOpenAPIToGraphQL
), so there would be no need to filter them.
But i think keeping this treatment in a separate function like processOperationCallbacks
allow to keep things clear in preprocessOas
.
@getlarge Actually, I still haven't fully reviewed the PR. Just wanted to let you know! Sorry, I've been bogged down by other work but so far, this looks very promising! Again, this is an incredible effort! I'm sorry I haven't been able to follow up as quickly as I'd like. |
@Alan-Cha It's ok, no worry! |
@Alan-Cha Hey, do you have any idea when you will have some time to work on this project again ? |
@getlarge Hey! I am more free this week. Will try to give you more feedback later today! Sorry for the long wait. Again, this is all highly appreciated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@getlarge Hey! I did a first pass and it looks very promising! The main thing I want to change is to store the callbacks in the PreprocessingData
. I think there's also a lot of code duplication so I would like to find a way to reduce that.
I still haven't run the code yet. I will look into that later today. I'm sorry that this is taking so long but I had to change my focus last month and I am really not familiar with pubsub and these different tools so I am learning as I go. Thanks for the patience!
Also keep in mind that these comments are not all directed at you, a lot of them are for me. I know this is still a work in progress so I have some comments just to remind me of things that need to be changed for the final version.
@@ -284,6 +284,7 @@ The following logging levels are supported: | |||
- `preprocessing`: Logs information about preprocessing the OAS. | |||
- `translation`: Logs information about translating an OAS to GraphQL. | |||
- `http`: Logs information about the HTTP requests made to the API. | |||
- `pubsub`: Logs information about the PubSub subscribptions made to the API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "subscriptions"
@@ -40,8 +40,10 @@ | |||
"types": "lib/index.d.ts", | |||
"scripts": { | |||
"api": "nodemon test/example_api_server.js", | |||
"api-5": "nodemon test/example_api5_server.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think we want to push this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it was to make my tests faster.
"dev": "tsc -w", | ||
"start_dev": "DEBUG=preprocessing,translation,http nodemon test/example_gql_server.js", | ||
"start_dev_2": "DEBUG=preprocessing,translation,http,pubsub nodemon test/example_gql_server2.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think we want to push this either.
@@ -107,7 +108,12 @@ export function createAndLoadViewer( | |||
} | |||
|
|||
// Create name for the viewer | |||
let viewerName = !isMutation | |||
let viewerName = isSubscription |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should refactor this piece of code so that it flows a bit better. Instead of if isSubscription
then if not isMutation
, maybe we should simply do if isSubscription
followed by isMutation
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we bring callbacks into a separate field of PreprocessingData
we won't need isSubscription
anymore.
@@ -136,10 +152,16 @@ export function createAndLoadViewer( | |||
} | |||
|
|||
// Create name for the AnyAuth viewer | |||
const anyAuthObjectName = !isMutation | |||
const anyAuthObjectName = isSubscription |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, refactor this so it is a bit clearer.
@@ -155,6 +355,17 @@ export function preprocessOas( | |||
// Links | |||
const links = Oas3Tools.getEndpointLinks(path, method, oas, data) | |||
|
|||
// Callbacks containing [key: string]:PathItemObject | |||
const callbacks = Oas3Tools.getEndpointCallbacks( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going back to this thought, I think callbacks
should just be another field in PreprocessingData
and we should populate it in preprocessOas
.
return customResolver.subscribe | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great touch! Nice attention to detail.
`Subscription schema : ${JSON.stringify(resolveData.usedPayload)}` | ||
) | ||
|
||
if (typeof resolveData.usedParams === 'undefined') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this also has to do with resolving links so it is candidate for removal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since resolveData.usedParams is used in the future resolveRuntimeExpression
, we will probably still need it.
args[paramNameWithoutLocation] = value | ||
} | ||
const topic = args[paramNameWithoutLocation] || 'test' | ||
pubsubLog(`Subscring to : ${topic}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
// */ | ||
if (value.search(/{|}/) === -1) { | ||
args[paramNameWithoutLocation] = isRuntimeExpression(value) | ||
? resolveLinkParameter(paramName, value, resolveData, root, args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function resolveLinkParameter
should probably be renamed to resolveRuntimeExpression
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure point, that's probably why you got misled in your previous comment.
But since the callback path is a runtime expression, like the link parameter ...
|
||
const TEST_PORT = 3000 | ||
const HTTP_PORT = 3005 | ||
const MQTT_PORT = 1885 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why this number was chosen in particular?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it was like, test number 5 ? so i append 5 to the port.
Not really deep reason ;)
Were you able to run your tests without failures? I just tried and encountered some issues. |
It seems to be a problem with overlapping port numbers with other test suites. |
From what i remember, all the tests were successful before i pushed. |
I addressed the issue by just changing the port number. The one other issue is that the Aedes MQTT server has some listeners that will try to log messages after the tests have finished running. I removed these listeners and I no longer have any more problems. |
Sorry for that, i also met the issues with the tests, my previous computer was probably too slow to encounter it ! ( port conflict and aedes listener ). I hope you appreciate this occasion to learn about pubsub communications. You mentioned code duplication, what do you have in mind ? |
@getlarge I have some basic questions that I would like to ask you to help my understanding. I'm not very experienced in this area which is part of the reason why my response is very slow. graphql-subscriptions works with different network transports and mentions subscriptions-transport-ws as an example but are you aware if there are any other network transports that are supported? I had trouble finding any other examples. If not, are you aware of any efforts to do so? I can see that this PR uses the key in the callbacks object as the topic of the pubsub connection. According to the documentation, “The key value used to identify the path item object is an expression, evaluated at runtime, that identifies a URL to use for the callback operation.” I wonder if the practice of using the key to define the topic of a pubsub connection instead of a URL is a common practice. |
On code duplication, don't worry about it. I'm currently making the changes. |
@Alan-Cha
If you want to see a rather simple example of using graphql subscriptions combined with an MQTT broker, you can take a look at this project : entourage. Concerning the callback object, using the path (as runtime expression) was the whole trick and it's not a common practice because it's not obvious. Originally the openAPI spec is not made for async communication, but by doing so we can resolve dynamically the topic and make it more compatible with pubsub protocols. |
@getlarge Thanks for clarification. I see that there are many other pubsub implementations that are available. This is good! We want to use something general and flexible. I think a code review would be very helpful! My colleague @ErikWittern also mentioned he would like to participate. In particular, there seems to be a problem with the test suite where there are asynchronous operations that have not been properly stopped when it ends. Maybe we can debug this together. In any case, what is a good time for you? I see. I think we should consider making this feature an option instead of default behavior for people who may have callbacks in the OAS for other reasons. Otherwise, they may see weird things in their schemas. It could be called something like |
@Alan-Cha The test suite is not really serious and profound to be honest, i just wanted to demonstrate the principles with a network configuration. And i totally agree on a Would you like to start this review with some projects using graphQL subscriptions ? |
@getlarge I see! We can discuss it in more detail tomorrow. I am in GMT -4 and @ErikWittern is GMT +2, I believe. Unfortunately he is offline right now but hopefully we can hear back from him tomorrow morning. I will also try to get up early so we can plan appropriately. My schedule is generally free except one meeting at 11 AM. Perhaps for now, we can tentatively plan for 9 AM for me, or 1 PM for you, @getlarge, and 2 PM for @ErikWittern. Does that work for you? We're very excited to discuss this with you! Thank you for your compliance! |
@Alan-Cha @ErikWittern I sent you an email. |
Signed-off-by: Alan Cha <[email protected]> Signed-off-by: getlarge <[email protected]>
Signed-off-by: getlarge <[email protected]>
Signed-off-by: getlarge <[email protected]>
Signed-off-by: getlarge <[email protected]>
1b8bbbb
to
921e21e
Compare
Finally, i left the long name
|
@getlarge I made a PR on your branch in which I reduced the amount of code duplication. To summarize, the preprocessing data now contains a new field I believe this can be pulled in to your branch. Let me know what you think. In the meantime, I will review the changes you've made since our last talk. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments regarding the documentation. I am happy to make these changes as they are a bit tedious. @getlarge Could you take a look at the PR I made on your branch? Once you accept it, I can make the appropriate changes and I will also try to rebase this entire PR.
First, initialize a PubSub instance to spread events between your API and the GraphQL Server, in a `pubsub.js` file. | ||
|
||
```javascript | ||
import { EventEmitter2 } from 'eventEmitter2'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this needs to be "eventEmitter2". Is the 2 necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessary, but eventEmitter2 allows to use wildcard in the topic chain. Which is very useful when you compose a topic when you have variable field that cannot be determined in advanced, but which are part of the topic structure. I mentioned it in the SUBSCRIPTION doc.
I have resolved most of the edits with the PR so I will mark them as resolved. |
@getlarge @ErikWittern I merged Edouard's PR with my PR and rebased it and submitted a new PR #311. I think we should continue the conversation there. |
Sorry for the delays. I had some deliverables for other work last week and I could not look into this at all. @getlarge Thanks for fixing the bug! I would actually recommend that we close this PR in favor of #311 as it has been properly rebased. I pulled over the changes you made and also made additional changes. Now I think we can finally get this in and make a new release! I would like @ErikWittern to do a final review, however. |
After reading #80, here is a proposal on a way to handle callbacks and create corresponding subscription fields.
There is only one test for now, i would like your feedback to know if you agree with the choices made, before expanding test phases.
Signed-off-by: Edouard Maleix [email protected]