algattik commented 2 years ago

Feature Request

The project should include a generated client for the Data Management API, which should be used in system tests (instead of RestAssured).

Which Areas Would Be Affected?

System tests

Greatly reduce the verbosity, and improve the clarity of system tests
Ensure the scope of system tests is not integration testing of the controllers, that can be done in specific RestAssured tests
Provide a client module in Maven that can be directly used by end users running on JVM
Dogfood our OpenAPI definition

We've had good results with openapi-generator-maven-plugin, the equivalent plugin exists for Gradle.

Improvement

algattik commented 2 years ago

@paullatzelsperger @ndr-brt @bscholtes1A a penny for your thoughts on this :)

ndr-brt commented 2 years ago

Having an EDC REST client would be great, I never got the chance to try the openapi generator, but looking forward for that.

algattik commented 2 years ago

@paullatzelsperger @ndr-brt @bscholtes1A @zeier I built a quick spike at https://github.com/agera-edc/DataSpaceConnector/pull/249. Happy for your comments and feedback on proposed decision points.

algattik commented 2 years ago

Develop a REST client module with generated code from the OpenAPI definition, and use it in system tests
Reduce the verbosity, increase the maintainability and improve the clarity of system tests
Ensure the scope of system tests is not integration testing of the controllers, that can be done in specific RestAssured tests
Provide a client module in Maven that can be directly used by end users running on JVM
Dogfood our OpenAPI definition

The org.openapi.generator plugin appears to work reliably as part of the build, and is fast (8 seconds).
The plugin generates an entire gradle project in the build/ directory. We have the option:
- Running the plugin manually and committing the files we want, as we are doing for the OpenAPI definition.
- Run the plugin as part of every build, and simply add the generated src/main/java folder to the source set.
Since the execution is fast, we opted for the latter option here.
In EDC, we standardise on OkHttpClient and Jackson. The plugin can use various HTTP client libraries and JSON serialisation frameworks. If generating for OkHttpClient, only GSON serialisation can be used. This means that new generated DTO classes must be used on the client side.

The generated DTO classes are mutable, and are created with this style: new MyDto().field1(value1).field2(value2)

An alternative would be to use the native Java HttpClient (Java 11+) and Jackson serialisation. We could then reuse the existing DTO classes. We would then have to refactor existing data management API modules to separate DTOs from services into separate packages.

Reuse existing DTOs or generate new DTOs? For quality purposes, my recommendation would be to avoid reusing DTO classes and use generated ones. This ensures that the client behaves exactly and only as specified in the OpenAPI definition, so that the Java client does not have any difference to any other client generated for another language.
Commit generated client code, or regenerate on build? My recommendation would be to regenerate the client on build. This keeps the repo lean, ensures the code is always up to date and does not get manually modified. The time impact is very modest, and should be hardly noticeable in practice thanks to gradle build parallelism and caching.
Choice of client library: My recommendation would be to use the native HttpClient rather than OkHttpClient to maximise portability and reduce client code size (OkHttpClient pulls in Kotlin). Clients who would want the additional advanced features of OkHttpClient would anyway be sophisticated enough that having to regenerate their own client from the OpenAPI definitions would hardly be an issue.

ndr-brt commented 2 years ago

@algattik Great work, at a first sight I like it, it took a little while to build everything and run the e2e test with it.

about the decisions:

I'm for using the generated DTOs, this will permit also to verify that the generated openapi is correct and that everything works correctly, as the client will be likely used from other teams to interact with EDC
I also like to regenerate code on build, at least for now that it does not take a lot, if we'll find this annoying we could switch to generated code later
regarding the client library probably the first gut feeling would be to use the okttp one since it's also the edc default httpclient, but, on the other hand, this client will be used separately from the EDC so having a lightweight library could be a first class feature. I would go for the latter.

algattik commented 2 years ago