Skip to content
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

Support openapi http proxy using openapi.yaml #778

Merged
merged 105 commits into from
Feb 23, 2024
Merged

Support openapi http proxy using openapi.yaml #778

merged 105 commits into from
Feb 23, 2024

Conversation

akrambek
Copy link
Contributor

@akrambek akrambek commented Feb 4, 2024

Description

Support openapi http proxy using openapi.yaml .

Fixes #740

akrambek and others added 30 commits October 25, 2023 13:35
…artial data frame while computing crc32c value
@@ -14,6 +14,7 @@
"dependencies":
[
"io.aklivity.zilla:binding-amqp",
"io.aklivity.zilla:binding-openapi",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are in alphabetical order, please move to sorted position.

*/
package io.aklivity.zilla.specs.binding.openapi;

public class OpenapiSpecs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be final with a private constructor.

Comment on lines 50 to 54
@Specification({
"${http}/reject.incorrect.origin/client",
"${http}/reject.incorrect.origin/server"
})
public void shouldRejectIncorrectOrigin() throws Exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend we rename this scenario to shouldRejectNonCompositeOrigin and scripts to reject.non.composite.origin.

Comment on lines 98 to 127
<dependency>
<groupId>io.aklivity.zilla</groupId>
<artifactId>binding-http</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>io.aklivity.zilla</groupId>
<artifactId>binding-tcp</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>io.aklivity.zilla</groupId>
<artifactId>binding-tls</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>io.aklivity.zilla</groupId>
<artifactId>catalog-inline</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>io.aklivity.zilla</groupId>
<artifactId>model-core</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>io.aklivity.zilla</groupId>
<artifactId>model-json</artifactId>
<version>${project.version}</version>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move all these default scope dependencies after provided scope and before test scope.

private static final String TLS_NAME = "tls";
private static final String HTTP_NAME = "http";
private static final String SPECS_NAME = "specs";
private OptionsConfigAdapter optionsAdapter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private OptionsConfigAdapter optionsAdapter;
private OptionsConfigAdapter tlsOptions;
private OptionsConfigAdapter httpOptions;

and a constructor

  public OpenapiOptionsConfigAdapter()
  {
      tlsOptions.adaptType("tls");
      httpOptions.adaptType("http");
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you mean in public void adaptContext since there you have access to context

Comment on lines 431 to 448
private SchemaView resolveSchemaForJsonContentType(
Map<String, MediaType> content,
OpenApi openApi)
{
MediaType mediaType = null;
if (content != null)
{
for (String contentType : content.keySet())
{
if (jsonContentType.reset(contentType).matches())
{
mediaType = content.get(contentType);
break;
}
}
}
return mediaType == null ? null : SchemaView.of(openApi.components.schemas, mediaType.schema);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helper method should go after all the injectXxx methods for improved readability.

@Specification({
"${http}/reject.incorrect.origin/client"
})
public void shouldRejectIncorrectOrigin() throws Exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldRejectNonCompositeOrigin()

Comment on lines 147 to 155
OpenApi openApi = null;
try (Jsonb jsonb = JsonbBuilder.create())
{
openApi = jsonb.fromJson(openapiText, OpenApi.class);
}
catch (Exception ex)
{
rethrowUnchecked(ex);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make sure we are using a validating schema to parse OpenAPI spec so we can communicate errors meaningfully.

Also, support OpenAPI spec 3.0, 3,1, perhaps also 2.0 since OpenAPI has been around for a long time?

Copy link
Contributor

@jfallows jfallows left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just the OpenAPI schema parsing left.

Recommend we store the openapi.3.0.schema.json in binding-openapi.spec so it can easily be reused from both binding-openapi and binding-openapi-asyncpi implementations.

@@ -173,7 +173,7 @@
</fileMappers>
</artifactItem>
</artifactItems>
<includes>io/aklivity/zilla/specs/binding/openapi/schema/openapi.schema.patch.json</includes>
<includes>io/aklivity/zilla/specs/binding/openapi/schema/*.json</includes>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this explicit, as we might decide to move the supporting openapi version-specific schemas to a different directory.

Suggested change
<includes>io/aklivity/zilla/specs/binding/openapi/schema/*.json</includes>
<includes>io/aklivity/zilla/specs/binding/openapi/schema/openapi.schema.patch.json</includes>
<includes>io/aklivity/zilla/specs/binding/openapi/schema/openapi.*.schema.json</includes>

Comment on lines 178 to 215
if (validateOpenapiSchema(openapiText))
{
openApi = jsonb.fromJson(openapiText, OpenApi.class);
try (Jsonb jsonb = JsonbBuilder.create())
{
openApi = jsonb.fromJson(openapiText, OpenApi.class);
}
catch (Exception ex)
{
rethrowUnchecked(ex);
}
}
return openApi;
}

private boolean validateOpenapiSchema(
String openapiText)
{
List<Exception> errors = new LinkedList<>();

boolean valid = false;

try
{
JsonValidationService service = JsonValidationService.newInstance();

String openApiVersion = detectOpenApiVersion(openapiText);
InputStream schemaInput = selectSchemaPathForVersion(openApiVersion);

JsonSchema schema = service.readSchema(schemaInput);
ProblemHandler handler = service.createProblemPrinter(msg -> errors.add(new ConfigException(msg)));

String readable = openapiText.stripTrailing();
Reader openapiReader = new StringReader(readable);

JsonReader reader = service.createReader(openapiReader, schema, handler);

JsonStructure json = reader.read();
valid = json != null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use JsonProvider provider = service.createJsonProvider(schema, problemHandler) to create a validating JsonProvider.
Then use JsonbBuilder.newBuilder().withProvider(provider).build() instead of Jsonb.create().

This will let you combine both methods into one and do a single pass of the the openapi document during (validating) parse / build.

*/
package io.aklivity.zilla.runtime.binding.openapi.internal.model;

public class BearerAuth
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public class BearerAuth
public class OpenApiBearerAuth

Please include OpenApi prefix on all model class names.

import io.aklivity.zilla.runtime.binding.openapi.internal.model.Operation;
import io.aklivity.zilla.runtime.binding.openapi.internal.model.ResponseByContentType;

public final class OperationView
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public final class OperationView
public final class OpenApiOperationView

Please include OpenApi prefix on all view class names.

@jfallows jfallows merged commit bf6be42 into aklivity:develop Feb 23, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support openapi http proxy using openapi.yaml
3 participants