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

OpenAPI definition endpoint #10328

Open
wants to merge 40 commits into
base: develop
Choose a base branch
from
Open

Conversation

jp-tosca
Copy link
Contributor

@jp-tosca jp-tosca commented Feb 16, 2024

What this PR does / why we need it:
On Dataverse 6.0 Payara was updated and this caused the url /openapi/ to stop working and caused an exception on the server.

This PR restores the functionality by adding a new endpoint /api/info/openapi/{format} this endpoint will take json/yaml as format and return the specification. Thanks to @poikilotherm for the recommendation to move the source files to the /META-INF/ directory.

We will need to address the following issues on the future:

  • The response of the API needs to be described with the @APIResponse annotation, otherwise they will not appear on the spec.
  • There is an issue with a couple of endpoints that share the same path: /api/v1/admin/groups/ip/{groupIdtf} with /api/v1/admin/groups/ip/{groupName} and /admin/workflows/{identifier} with /admin/workflows/{id} these endpoints share the same path but one of them uses PUT another GET so we may have to find a solution and decide what kind of solution we do for these endpoints, also keep in mind when creating new endpoints that "paths must be unique".
  • There are some other issues but I am still getting a grasp of what they are, some are just warnings but beside the 2 duplications are another 6 errors but it will all come down on how much time/effort wants to be put into improving the spec.

I think the most important takeaway of now would be that if you create an endpoint be sure the path is unique also if you can start adding the @APIResponse annotation to new endpoints it would be great 😄

We incorporated the SmallRye OpenAPI plugin on our POM (https://github.com/smallrye/smallrye-open-api/tree/main/tools/maven-plugin) which will generate files for YAML and JSON formats and deposit them on edu/harvard/iq/dataverse/openapi/ these files will be provided by this endpoint depending on the format requested.

Which issue(s) this PR closes:

Special notes for your reviewer:

We are still waiting to hear back from @JR-1991 since he is helping with the validation. The PR is ready for review but we are keeping it as Draft to prevent from anyone merging it until we hear back from Jan.

Suggestions on how to test this:

A new endpoint was added at /api/info/openapi/{format} where version can be json or yaml, the parameter is not case sensitive so it will take YaMl or JsOn. It will allow in the future to add more formats if they are added.

The maven plugin added will generate the files.

Is there a release notes update needed for this change?:
Yes

Updated on Apr 17 2024: Payara has fixed the original endpoint but it is generating a spec definition that still has errors due duplicated names. We did some testing and regardless of what is on the documentation, the server is not returning the provided files on the META-INF directory, we tested this on the application root META-INF and the currently used META-INF. At this point we decided to use this solution that will override the original endpoint and server the generated files from the smallrye plugin.

Preview of docs: https://dataverse-guide--10328.org.readthedocs.build/en/10328/api/native-api.html#get-openapi-specification

@jp-tosca jp-tosca self-assigned this Feb 16, 2024
@jp-tosca jp-tosca added the Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) label Feb 16, 2024
@jp-tosca jp-tosca changed the title Openapi definition endpoint 10236 Openapi definition endpoint Feb 16, 2024
@coveralls
Copy link

coveralls commented Feb 16, 2024

Coverage Status

coverage: 20.57% (-0.009%) from 20.579%
when pulling 9ca4111 on openapi-definition-endpoint
into dae5ca7 on develop.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

I assume docs are coming. Just a quick initial review. I'm excited that SmallRye is working to create the OpenAPI output!

jp-tosca and others added 3 commits February 16, 2024 14:50
Co-authored-by: Philip Durbin <philip_durbin@harvard.edu>
Co-authored-by: Philip Durbin <philip_durbin@harvard.edu>
Co-authored-by: Philip Durbin <philip_durbin@harvard.edu>

This comment has been minimized.

5 similar comments

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

A little more doc review.

doc/sphinx-guides/source/api/changelog.rst Outdated Show resolved Hide resolved
doc/sphinx-guides/source/api/native-api.rst Outdated Show resolved Hide resolved
doc/sphinx-guides/source/api/native-api.rst Outdated Show resolved Hide resolved
doc/sphinx-guides/source/api/native-api.rst Outdated Show resolved Hide resolved

This comment has been minimized.

2 similar comments

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@jp-tosca jp-tosca removed their assignment Feb 16, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@jp-tosca jp-tosca added the Status: Waiting issues in need of input from someone currently unavailable label Apr 4, 2024
@scolapasta scolapasta removed the Status: Waiting issues in need of input from someone currently unavailable label Apr 16, 2024
@jp-tosca jp-tosca removed their assignment Apr 17, 2024

This comment has been minimized.

@jp-tosca
Copy link
Contributor Author

jp-tosca commented May 14, 2024

14/05/2024: Branch updated and resolved conflicts 😮‍💨

This comment has been minimized.

@pdurbin pdurbin self-assigned this May 29, 2024

This comment has been minimized.

1 similar comment

This comment has been minimized.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

I'm not done reviewing but I pushed some doc changes and left a few comments.

The description of this pull request seems out of date, the URL paths, especially. Can it please be updated?

} else {
List<String> args = Arrays.asList(format);
String bundleResponse = BundleUtil.getStringFromBundle("openapi.exception.invalid.format", args);
resp.sendError(Response.Status.UNSUPPORTED_MEDIA_TYPE.getStatusCode(),
Copy link
Member

Choose a reason for hiding this comment

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

This error is being returned in xhtml. Can we please return JSON instead?

For example: http://localhost:8080/openapi?format=foobar

<?xml version="1.0"?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
    <title>Payara Server 6.2023.8 #badassfish - Error report</title>
    <style type="text/css">
<!--H1 {font-family:Tahoma,Arial,sans-serif;color:white;background-color:#525D76;font-size:22px;} H2 {font-family:Tahoma,Arial,sans-serif;color:white;background-color:#525D76;font-size:16px;} H3 {font-family:Tahoma,Arial,sans-serif;color:white;background-color:#525D76;font-size:14px;} BODY {font-family:Tahoma,Arial,sans-serif;color:black;background-color:white;} B {font-family:Tahoma,Arial,sans-serif;color:white;background-color:#525D76;} P {font-family:Tahoma,Arial,sans-serif;background:white;color:black;font-size:12px;}A {color : black;}HR {color : #525D76;}-->
    </style>
  </head>
  <body>
    <h1>HTTP Status 415 - Invalid format foobar, currently supported formats are YAML and JSON.</h1>
    <hr />
    <p><b>type</b> Status report</p>
    <p><b>message</b>Invalid format foobar, currently supported formats are YAML and JSON.</p>
    <p><b>description</b>The server refused this request because the request entity is in a format not supported by the requested resource for the requested method.</p>
    <hr />
    <h3>Payara Server 6.2023.8 #badassfish</h3>
  </body>
</html>

.body(equalTo(jsonDefinitionFileContent));

String yamlFileContent = UtilIT.getDatasetJson("src/main/resources/edu/harvard/iq/dataverse/openapi/dataverse_openapi.yaml");
response = given().get("/api/info/openapi/yaml");
Copy link
Member

Choose a reason for hiding this comment

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

Are these the right paths? /api/info/openapi instead of /openapi?

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 am removing this test from here OpenApiIT was created for this.

This comment has been minimized.

Copy link

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:openapi-definition-endpoint
ghcr.io/gdcc/configbaker:openapi-definition-endpoint

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: API Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) Type: Bug a defect
Projects
Status: In Review 🔎
Development

Successfully merging this pull request may close these issues.

Spike: Investigate creating OpenAPI YAML from our code OpenAPI endpoint is broken
5 participants