-
Notifications
You must be signed in to change notification settings - Fork 93
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
Develop OpenAPI Export for API Definition #1178 #1202
Conversation
Fixing as per jdegoes's comments
Honestly, all of these already maintaned openapiv3 crates are extremely problematic. Let alone documentation, they dont even have the correct forked customs so its extremely hard to build with just them. It might be better to use our custom openapi3 types and use them as validation only.
The code has been updated to incorporate feedback from @jdegoes, utilizing a custom OpenAPIv3 implementation and glademiller's crate for validation. Further review and feedback are appreciated. |
golem-worker-executor-base/src/durable_host/blobstore/container.rs
Outdated
Show resolved
Hide resolved
golem-worker-executor-base/src/durable_host/blobstore/container.rs
Outdated
Show resolved
Hide resolved
golem-worker-executor-base/src/durable_host/filesystem/types.rs
Outdated
Show resolved
Hide resolved
golem-worker-executor-base/src/durable_host/filesystem/types.rs
Outdated
Show resolved
Hide resolved
- Made sure to use newest component_name - Added security documentation - Used TryFrom - Updated the redis
Pushed the commit according to latest reviews. |
params.extend(Self::extract_header_parameters(route)); | ||
if params.is_empty() { None } else { Some(params) } | ||
}, | ||
request_body: Self::create_request_body(route), |
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 the "main" binding type whose path parameters, query parameters, request body, and response body, are all known and can be encoded precisely through conversion of Rib typing information.
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, that is correct. The BindingType::Default handles comprehensive parameter binding and type conversion. Specially:
- Extracts and converts path parameters from the route path
- Handles query parameters from the route definition
- Processes header parameters
- Converts request/response bodies using the Rib type system
- All parameter types are precisely encoded through the OpenAPI converter, maintaining type safety from Golem's internal types to the OpenAPI spec.
Based on jdegoes's comments, could be improved further by adding more error documentation if needed.
Added more detailed documentation. |
r#in: ParameterLocation::Path, | ||
description: None, | ||
required: Some(true), | ||
schema: if info.key_name.ends_with("_id") { |
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.
For path parameters, Rib has inferred types, which we should be using to generate the schema.
In general, we will need, in a separate module (so it's easy to test), a way to convert a Rib type into an OpenAPI schema. Then we will use that for the path parameters, query variables, and request / response headers & bodies.
if route.path.contains("/workers") && route.method == HttpMethod::Get { | ||
params.push( | ||
Parameter { | ||
name: "filter".to_string(), |
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.
See comment on Rib type conversion required here.
} | ||
); | ||
} | ||
if route.path.contains("/invoke-and-await") || route.path.contains("/invoke") { |
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.
What is the point of this special-case logic here? Also, I am not sure what this is trying to do because in general, we should be generically converting the user-defined API into OpenAPI (not our own REST API that we expose for workers). Perhaps this is arising from the reuse of the same set of functions across all binding types--if so we should use different functions for different binding types because there's no way to unify them at this level.
} | ||
); | ||
} | ||
if route.path.contains("/api/definitions") && route.method == HttpMethod::Get { |
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.
Again, all this special-case logic doesn't begin here. We want to generate OpenAPI for user-defined APIs (which are defined with file server, swagger, and default / worker binding types), not for Golem REST APIs.
|
||
fn extract_header_parameters(route: &Route) -> Vec<Parameter> { | ||
let mut params = Vec::new(); | ||
if route.path.contains("/invoke-and-await") || route.path.contains("/invoke") { |
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 am convinced all this special-case logic around the Golem REST APIs is not correct and not satisfying the requirements of the ticket, which are to convert the user-defined API into OpenAPI schema.
params | ||
} | ||
|
||
fn create_request_body(route: &Route) -> Option<RequestBody> { |
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 implementation should be re-evaluated in light of the above comments.
Essentially, what you need to be doing here is looking at the inferred query, path, request, and response types (which is something you get from the Rib compiler metadata associated with worker binding type), and then generating OpenAPI for them.
That's only for worker binding type. For file server, it's going to be much simpler, and for swagger UI binding type, it will be even simpler. The most complex type is when a user has defined a route with Rib.
@zelosleone I stopped reviewing for now because I discovered the core of the problem is not solved, which is: when the user builds a custom API, using Rib (worker binding), file server, and of course the new Swagger UI binding types for different routes, an OpenAPI Schema needs to be generated that precisely describes how to interact with this API (including CORS and Security, and having full information on request and response, including headers, bodies, and query and path variables). As far as I can tell, the essential logic to go from a Rib inferred type to some element of OpenAPI schema is not present, which means this solution does not satisfy the requirements. However, you overall have the right architecture and are looking in the right places, so I look forward to taking another look when you have a chance to address these issues. Please re-open when re-architected! |
API Definition OpenAPI Export & Swagger UI Integration
Key Implementations
OpenAPI Export
Swagger UI Integration
Testing
Technical Details
Key Features
CI Configuration Updates:
Dependency Updates:
heck
andvalidator
and updated thewasmtime
dependencies to version21.0.1
inCargo.toml
. [1] [2]golem-wit
dependency version to1.1.0
ingolem-worker-executor-base/Cargo.toml
.Code Refactoring:
ParsedFunctionSite
andParsedFunctionReference
enums to a new location ingolem-rib/src/function_name.rs
and added a newParsedFunctionName
struct. [1] [2] [3] [4] [5] [6]clone
method toinit_tracing
call ingolem-cli/src/lib.rs
.DurableWorkerCtx
ingolem-worker-executor-base/src/durable_host/blobstore/container.rs
. [1] [2] [3] [4] [5] [6] [7]This implementation provides seamless integration between Golem's API definitions and standard OpenAPI tooling while maintaining all the power of Golem's type system and binding features.
Docs Pull Request: golemcloud/docs#105
/claim #1178