fix OpenAPI definition (and thus making all language-specific SDKs obsolete) #94
Replies: 12 comments 22 replies
-
|
anyway, going to start on this when i have the time; probably start with fixing the sheet endpoints (since those are the ones i use most) and then expand it to the other public endpoints as time/energy allows. not sure if there exists a public announcement mechanism for up and coming changes to the API but, gee wiz, wouldn't that be something an enterprise-level company like Smartsheet should provide? |
Beta Was this translation helpful? Give feedback.
-
|
Hey @ronreynolds, sorry just seeing these discussions. They seem like they will be perfect for communicating. I think @apederson94 or @DonoA looked at generating our SDK based on the OpenAPI spec and ran into some challenges. Maybe they can chime in. |
Beta Was this translation helpful? Give feedback.
-
|
i know the OpenAPI definition as it exists with our API docs is unusable - huge, heavy with documentation taken straight from Keven F's docs, etc. so my thinking was to build them by hand (yes, crazy, but at least it would be a starting point) to at least handle the sheet endpoints. pretty sure the model is sane (or at least sane enough) because it's Jackson on both ends and i don't remember anything server-side that GENERATED insane JSON (tho i do remember some rather fancy annotations for enum-handling and a few other bits on the receiving side but that's all within the API layer of app-core or the services now handling those endpoints). so i guess where i would start is the model. i also believe the API endpoints take quite the variety of URL query-string params (more so, i think, than a "proper REST" API would) but i don't think that would make it un-openapi-able; just somewhat unsavory from an API-purist POV. |
Beta Was this translation helpful? Give feedback.
-
|
under way - https://github.com/ronreynolds/smartsheet-openapi
|
Beta Was this translation helpful? Give feedback.
-
|
Hey sorry, just seeing this now. So there were a few key challenges I ran in to while attempting to make the swagger generator work on the open api spec:
There were a number of other issues related to the parser itself, however I think a full rewrite will correct these. |
Beta Was this translation helpful? Give feedback.
-
|
I think we can solve these problems if we build iteratively and find where the generator is insufficient, however it was too monumental of a task for me to want to undertake 😬 |
Beta Was this translation helpful? Give feedback.
-
|
yes, i presumed an iterative approach will be needed too (i.e., remove (for now) those endpoints that can't be supported via OpenAPI and then see if they can be finagled to work with OpenAPI later). at the moment i'm running into a weird issue with parameter lists that contain multiple refs to other endpoints parameters; the short-term work-around was to replace the refs with a copy of the parameter referenced but hopefully that's not long-term required (tho the only reason i can think of to use refs is to save on disk space and, like any ref, it opens up the possibility of inadvertently breaking one endpoint by changing another so perhaps normalizing all those refs is the best long-term solution (i.e., each endpoint definition is a stand-alone)). thankfully i have more time now that i'm not doing Smartsheet day-to-day stuff. |
Beta Was this translation helpful? Give feedback.
-
If I'm not mistaken this is a bug in the swagger parser I also ran into. Ref tags aren't processed correctly when enclosed in certain parts of the spec. I also fixed this by simply writing a script which copied the content of these files into the parent.
I wonder how true this is under the hood. A good number of models are shared within grid for each of it's endpoints.
I can only dream |
Beta Was this translation helpful? Give feedback.
-
currently dealing with query-string parameters (all of which are, so far, primitives). not sure one can have multiple schema |
Beta Was this translation helpful? Give feedback.
-
|
been building a draw.io model of the Etsy OpenAPI spec (for my other work) and have finally circled back to Smartsheet's OpenAPI spec; creating a model for this API would be insane because complex types are defined WITHIN the endpoint definitions (which are then referenced by other endpoint definitions). no wonder the parser can't handle it. i also noticed that the ID types (which are longs by nature) are specified as type "number" so need to update all of those. |
Beta Was this translation helpful? Give feedback.
-
|
Haha I would certainly have less motivation if I wasn't being paid by the company. Glad you're still around and willing to help out the SDK though |
Beta Was this translation helpful? Give feedback.
-
|
FYI (for those who are interested in OpenAPI specs) there is literally NO difference (according to the generated code) between this: and this: |
Beta Was this translation helpful? Give feedback.
-
|
i noticed in the parameters of the listEvents endpoint (GET /events) but i don't see it in the guessing this is a case where the API has gotten ahead of the SDK? |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, it certainly seems like it. We are supposed to have a meeting in the next week or so with leadership to discuss the goals of the SDK and the direction we want to take it |
Beta Was this translation helpful? Give feedback.
-
|
well, FWIW, 2 days ago i had a subset of the paths (and all of the schema) parse and compile. rather than test it i decided to throw the rest of the paths back in so it doesn't parse again but i'm working thru all the |
Beta Was this translation helpful? Give feedback.
-
|
still going thru all the |
Beta Was this translation helpful? Give feedback.
-
|
@ronreynolds , I ended up writing a script to relocate and remap all the REF tags when I did this I believe because the import paths were causing substantial issues. IIRC, there are legitimate bugs in how REF tags are handled in the Java Generator project where they get resolved differently based on location. |
Beta Was this translation helpful? Give feedback.
-
|
@DonoA i think i'm starting to find some of the issues you mentioned above; for instance requests like this: "requestBody": {
"content": {
"application/json": {
"schema": {
"oneOf": [
{
"$ref": "#/components/schemas/GroupMember"
},
{
"type": "array",
"items": {
"$ref": "#/components/schemas/GroupMember"
}
}
]
}
}
}current generate Java code that can't compile; e.g., if (List<GroupMember>.class.equals(Integer.class) || List<GroupMember>.class.equals(Long.class) || List<GroupMember>.class.equals(Float.class) || List<GroupMember>.class.equals(Double.class) || List<GroupMember>.class.equals(Boolean.class) || List<GroupMember>.class.equals(String.class)) {wondering if it's possible to coerce the generator to use actual types rather than |
Beta Was this translation helpful? Give feedback.
-
|
one thought that comes to mind to address this is that the API server should be able to handle an array of 1 so why make the distinction (or support if it's a PITA) sending both single and an array of elements? not sure it'll address this but it will simplify things somewhat and there's no reason for a client to support EVERY use-case (at least not use-cases like "supports a single element or an array of 1 or more elements...") |
Beta Was this translation helpful? Give feedback.
-
|
this approach is definitely solving the compile-errors (replace |
Beta Was this translation helpful? Give feedback.
-
|
oneOf was a major source of pain and AFAICT, the java generator just doesn't have great support for this. If possible, I think we should just get rid of our oneOf models, but in lieu of that, I was planning to investigate modifying the spec to use inheritance instead of oneOf. Since the JSON has no class information I assume this would have no impact on the actual request/response objects. |
Beta Was this translation helpful? Give feedback.
-
|
Hey @ronreynolds, I remember that I was going to do some work on figuring out how to replace a lot of the code in our repo with Retrofit. I don't remember who it was, but someone commented on how we have given users of our SDK the ability to pass in their own HTTP clients and how we don't necessarily want to remove that feature. I'm curious on your thoughts about this and how we'd be able to get some of this functionality still with the OpenAPI generated code |
Beta Was this translation helpful? Give feedback.
-
|
oh absolutely you can choose the HTTP client and JSON processor to use with openapi-generated code since the spec file is a pure model of the API endpoints, requests, and responses (and a few other bits of behavior). where the spec becomes real code has gobs of configurability (almost too much but, ironically, never enough. ROFL). these are the options with the Java open-api generator - https://openapi-generator.tech/docs/generators/java#config-options |
Beta Was this translation helpful? Give feedback.
-
|
I think this was partially a function of the clients wanting to be able to put their own spin on the HTTP clients at runtime (i.e. they want a different timeout or retry count or whatever). I couldn't seem to parse out whether this is possible with the OpenAPI generated code. Is that something that is still possible? |
Beta Was this translation helpful? Give feedback.
-
|
@apederson94 you CHOOSE the http-client library to use when you convert the openapi spec JSON into code. i'm not sure if openapi-generated code allows for a runtime-pluggable http-client library but i suppose if you generate the code with a http-client that is pluggable it could, in theory, be supported. |
Beta Was this translation helpful? Give feedback.
-
|
@ronreynolds , would having an additional set of hands be helpful with this project? When I was investigating this it was mostly squashing bugs one at a time. |
Beta Was this translation helpful? Give feedback.
-
|
@DonoA you can see what i've done thus far at https://github.com/ronreynolds/smartsheet-openapi/tree/work/ron/add-back-paths so now i need to pick out the changes until this issue is fixed (the stacktrace doesn't help AFAICT): once i get this worked out i'll push the latest openapi spec. and if i can get past the last 6 build errors (sans the above) i'll merge it into main. |
Beta Was this translation helpful? Give feedback.
-
|
@ronreynolds I can't say if this is related but I ran in to circular reference issues when working with the spec. I believe I was able to use this script to fix it: https://gist.github.com/DonoA/29e3466b5784d4c014f7fe58a0cefb45 Either way it may be helpful as a reference. |
Beta Was this translation helpful? Give feedback.
-
|
@DonoA fixed it by rolling back and then reapplying changes incrementally; i think it might have been an empty schema type (that's the only thing i found in the broken version that looked very wrong). down to 6 compiler errors left; very optimistic that it'll work this week. :) |
Beta Was this translation helpful? Give feedback.
-
|
@ronreynolds , glad to hear it. Sounds like you're making good progress which is impressive because the error messages as I recall and basically worthless. I ended up downloading the source and running it locally to use the debugger to step though for better understanding of the error. |
Beta Was this translation helpful? Give feedback.
-
|
@DonoA i actually find the output full of (overflowing) with info but typically the last entries are lacking; more interesting are errors like |
Beta Was this translation helpful? Give feedback.
-
|
@DonoA @apederson94 |
Beta Was this translation helpful? Give feedback.
-
|
and i came up with a potential HACK work-around that let me put the responses back in the spec file. it's not pretty but hacks often aren't. |
Beta Was this translation helpful? Give feedback.
-
|
invited you all to my openapi project; also here's an MR for your consideration - ronreynolds/smartsheet-openapi#5 |
Beta Was this translation helpful? Give feedback.
-
been working with a few APIs built from OpenAPI definition files which provide an extreme level of flexibility (language, frameworks, underlying HTTP transport lib, etc) and wondering what's the point of maintaining a language-specific SDK when one can be generated using a build plugin like
org.openapi.generator? the "only" requirement is that we fix the OpenAPI definition file at https://smartsheet.redoc.ly/. unfortunately last i checked that file was totally borked and i know this is a huge task but imagine the benefits if we could actually get it to work!Beta Was this translation helpful? Give feedback.
All reactions