Repository navigation
Conversation
fjtirado
force-pushed
the
Fix_1778
branch
9 times, most recently
from
October 27, 2026 09:54
3b967c6 to
5f0090f
Compare
fjtirado
marked this pull request as ready for review
October 27, 2026 09:56
fjtirado
requested review from
gmunozfe,
mathecruz and
ricardozanini
and
a balanced review from Copilot
October 27, 2026 09:56
fjtirado
marked this pull request as draft
October 27, 2026 09:59
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
HTTP correctness regressions and a resource-loading deadlock with bounded executors remain unresolved.
11 open issues
What changed in this PR
Refactors HTTP handling for #1778 so external resources can reuse configured workflow/task clients, while retaining a JDK-based transport without Jakarta dependencies.
Changes:
- Introduces shared HTTP request/response abstractions and integrates resource downloads.
- Moves HTTP task support and default URI-template resolution into core, with optional Jakarta implementations.
- Updates authentication interfaces, JSON model conversion, service registrations, and tests.
| File | Description |
|---|---|
| README.md | Adds URI-template code attribution. |
| impl/test/src/test/java/org/openworkflow/sdk/impl/test/OpenAPITest.java | Updates cache headers and assertions. |
| impl/test/src/test/java/org/openworkflow/sdk/impl/test/HTTPWorkflowDefinitionTest.java | Exercises the JDK transport. |
| impl/test/src/test/java/org/openworkflow/sdk/impl/test/CustomAuthProviderFactoryOverrideTest.java | Adapts custom authentication signatures. |
| impl/test/src/test/java/org/openworkflow/sdk/impl/auth/OAuthRequestBuilderTest.java | Uses renamed authentication request type. |
| impl/template/src/main/resources/META-INF/services/org.openworkflow.sdk.impl.resources.URITemplateResolver | Removes old resolver registration. |
| impl/template/pom.xml | Removes standalone template module. |
| impl/README.md | Updates module descriptions. |
| impl/pom.xml | Removes template module configuration. |
| impl/openapi/src/main/java/org/openworkflow/sdk/impl/executors/openapi/OpenAPIExecutorBuilder.java | Uses shared request builder. |
| impl/openapi/src/main/java/org/openworkflow/sdk/impl/executors/openapi/OpenAPIExecutor.java | Executes through shared HTTP abstractions. |
| impl/model/src/main/java/org/openworkflow/sdk/impl/model/jackson/JacksonModelFactory.java | Adds JSON-byte deserialization. |
| impl/model/src/main/java/org/openworkflow/sdk/impl/model/jackson/JacksonModel.java | Adds JSON-byte serialization. |
| impl/jwt-impl/src/main/java/org/openworkflow/sdk/impl/executors/http/jackson/JacksonModelConverterFactory.java | Updates Jakarta converter imports. |
| impl/jackson/pom.xml | Removes standalone template dependency. |
| impl/http/src/main/resources/META-INF/services/org.openworkflow.sdk.impl.http.WorkflowHttpRequestFactory | Registers Jakarta request factory. |
| impl/http/src/main/resources/META-INF/services/org.openworkflow.sdk.impl.http.URITemplateResolver | Registers relocated Jakarta resolver. |
| impl/http/src/main/resources/META-INF/services/org.openworkflow.sdk.impl.executors.CallableTaskBuilder | Removes HTTP-module task registration. |
| impl/http/src/main/resources/META-INF/services/org.openworkflow.sdk.impl.auth.AccessTokenProviderFactory | Updates token-provider registration. |
| impl/http/src/main/java/org/openworkflow/sdk/impl/jakarta/template/JaxrsURITemplateResolver.java | Relocates Jakarta template resolver. |
| impl/http/src/main/java/org/openworkflow/sdk/impl/jakarta/http/JakartaRequestExecutorFactory.java | Creates Jakarta requests. |
| impl/http/src/main/java/org/openworkflow/sdk/impl/jakarta/http/JakartaRequestExecutor.java | Implements Jakarta request execution. |
| impl/http/src/main/java/org/openworkflow/sdk/impl/jakarta/http/JakartaHttpResponse.java | Adapts Jakarta responses. |
| impl/http/src/main/java/org/openworkflow/sdk/impl/jakarta/http/HttpRequestDecorator.java | Relocates request decorator interface. |
| impl/http/src/main/java/org/openworkflow/sdk/impl/jakarta/http/HttpModelConverter.java | Relocates and simplifies conversion interface. |
| impl/http/src/main/java/org/openworkflow/sdk/impl/jakarta/http/HttpConverterResolver.java | Supports optional execution contexts. |
| impl/http/src/main/java/org/openworkflow/sdk/impl/jakarta/http/HttpClientResolver.java | Resolves clients with optional contexts. |
| impl/http/src/main/java/org/openworkflow/sdk/impl/jakarta/auth/JaxRSAccessTokenProviderFactory.java | Passes application into token providers. |
| impl/http/src/main/java/org/openworkflow/sdk/impl/jakarta/auth/JaxRSAccessTokenProvider.java | Adapts token requests to optional contexts. |
| impl/http/src/main/java/org/openworkflow/sdk/impl/executors/http/WithoutBodyRequestExecutor.java | Removes superseded bodyless executor. |
| impl/http/src/main/java/org/openworkflow/sdk/impl/executors/http/WithBodyRequestExecutor.java | Removes superseded body executor. |
| impl/http/src/main/java/org/openworkflow/sdk/impl/executors/http/HttpExecutorBuilder.java | Removes old Jakarta-specific builder. |
| impl/http/src/main/java/org/openworkflow/sdk/impl/executors/http/HttpExecutor.java | Removes old HTTP-module executor. |
| impl/http/src/main/java/org/openworkflow/sdk/impl/executors/http/AbstractRequestExecutor.java | Removes superseded execution base. |
| impl/http/pom.xml | Depends directly on core and Jakarta. |
| impl/core/src/main/resources/META-INF/services/org.openworkflow.sdk.impl.executors.CallableTaskBuilder | Registers HTTP tasks in core. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/WorkflowValueResolver.java | Adds optional-context resolution. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/WorkflowUtils.java | Updates secrets and template access. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/WorkflowModelFactory.java | Adds optional JSON deserialization contract. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/WorkflowModel.java | Adds optional JSON serialization contract. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/WorkflowError.java | Supports errors without task context. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/WorkflowDecorator.java | Documents context nullability. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/WorkflowApplication.java | Configures HTTP factories and resolver defaults. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/resources/ResourceLoader.java | Passes authentication and execution contexts. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/resources/HttpResource.java | Downloads through configured HTTP factory. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/resources/DefaultResourceLoader.java | Propagates contexts into HTTP resources. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/http/WorkflowHttpResponse.java | Defines shared response contract. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/http/WorkflowHttpRequestInfoBuilderFactory.java | Resolves workflow request configuration. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/http/WorkflowHttpRequestInfo.java | Defines request data and builder. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/http/WorkflowHTTPRequestHelper.java | Centralizes authentication and response handling. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/http/WorkflowHttpRequestFactory.java | Defines pluggable request factory. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/http/WorkflowHttpRequest.java | Defines transport-neutral request invocation. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/http/URITemplateResolver.java | Relocates resolver contract. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/http/DefaultURITemplateResolver.java | Adds dependency-free template expansion. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/http/DefaultHttpResponse.java | Adapts JDK responses. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/http/DefaultHttpRequestBuilderFactory.java | Provides fallback JDK client. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/http/DefaultHttpRequest.java | Implements JDK request execution. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/executors/http/HttpExecutor.java | Executes HTTP tasks through shared factory. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/executors/http/CallableTaskHttpExecutorBuilder.java | Builds transport-neutral HTTP tasks. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/auth/HttpRequestInfoBuilder.java | Builds renamed authentication request type. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/auth/DigestAuthProvider.java | Uses shared HTTP for digest challenges. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/auth/CommonOAuthProvider.java | Adapts OAuth resolution and factory calls. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/auth/BearerAuthProvider.java | Supports optional authentication contexts. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/auth/BasicAuthProvider.java | Supports optional authentication contexts. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/auth/AuthRequestBuilder.java | Updates authentication request return type. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/auth/AuthProvider.java | Revises authentication context contract. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/auth/AuthHttpRequestInfo.java | Renames authentication request record. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/auth/AccessTokenProviderFactory.java | Adds application to factory contract. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/auth/AccessTokenProvider.java | Accepts optional execution contexts. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/auth/AbstractAuthRequestBuilder.java | Returns renamed authentication request type. |
| impl/core/src/main/java/org/openworkflow/sdk/impl/additional/WorkflowAdditionalObject.java | Defines nullable-context factory contract. |
Review effort: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Resource loading can deadlock on bounded executors, and redirect handling and conditional-request dates remain incorrect.
10 open issues
4 resolved since last review
Review effort: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved request-construction and serialization defects can alter HTTP requests or reject valid workflow inputs.
10 open issues
6 resolved since last review
Review effort: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Request encoding, conditional headers, and response-error translation contain unresolved defects affecting HTTP behavior and workflow recovery.
14 open issues
Review effort: Balanced
fjtirado
commented
Oct 27, 2026
Collaborator
Author
Body in is considered here |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The change silently drops Swagger 2 free-form request bodies and reuses GET Digest credentials for HEAD revalidation, causing authentication failures and repeated schema downloads.
1 open issue
1 resolved since last review
Review effort: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Eager HTTP client initialization can break application construction without a provider, while unsynchronized shared ServiceLoader access can cause parallel requests to fail or miss decorators.
2 open issues
2 resolved since last review
Review effort: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
When both contexts are present, converter lookup ignores additionalObjectSuppliers, causing HTTP/OpenAPI tasks to fall back to Object.class and lose direct JsonNode conversion.
1 open issue
2 resolved since last review
Review effort: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The contextual lookup throws when a workflow has no task, preventing client or supplier fallback, and skips registered providers when both contexts are absent despite their documented null-context support.
1 open issue
1 resolved since last review
Review effort: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
HTTP shutdown can block indefinitely because task timeouts leave underlying exchanges active, and the new default URI-template resolver is not exercised by the existing tests.
1 open issue
1 resolved since last review
Review effort: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A shared, non-thread-safe ServiceLoader risks failed or incomplete decorator discovery during concurrent Jakarta requests, and malformed HTTP dates can cause servers to ignore or reject conditional requests.
2 open issues
1 resolved since last review
Review effort: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The HTTP date formatter uses the JVM’s default locale, which can produce invalid If-Modified-Since headers under non-English locales; specify Locale.US explicitly.
1 open issue
2 resolved since last review
Review effort: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Shared OAuth/OIDC request-builder state can mix credentials across overlapping token exchanges, while JacksonModelCollection request bodies fail before transport because they lack an asJson() override.
2 open issues
1 resolved since last review
Review effort: Balanced
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The cached OAuth/OIDC provider shares mutable credential maps across token exchanges, allowing overlapping requests during rotation to send new secrets to an old issuer.
No open issues 🎉
2 resolved since last review
Review effort: Balanced
mathecruz
reviewed
Oct 29, 2026
mathecruz
reviewed
Oct 29, 2026
Signed-off-by: Francisco Javier Tirado Sarti <ftirados@ibm.com>
Signed-off-by: Francisco Javier Tirado Sarti <ftirados@ibm.com>
…Factory to content Signed-off-by: Francisco Javier Tirado Sarti <ftirados@ibm.com>
Signed-off-by: Francisco Javier Tirado Sarti <ftirados@ibm.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The changes bypass custom authentication when no explicit policy is supplied and can deadlock OpenAPI schema downloads on single-thread or saturated executors.
1 open issue
4 resolved since last review
Review effort: Balanced
Signed-off-by: Francisco Javier Tirado Sarti <ftirados@ibm.com>
…rameters Signed-off-by: Francisco Javier Tirado Sarti <ftirados@ibm.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Requests without an authentication policy can lose factory-provided credentials, and reserved URI template expansions emit illegal characters without encoding, causing URI creation to fail.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix #1778
AuthProviderFactory was changed (method was moved from getAuth to content). Thats breaks backward compatibility, but this will be part of a major release, so it is the right momento to fix that flaw.