REST data Panache reorganisation #9373

gytis · 2020-05-16T12:10:15Z

Sorry for a big PR. I wanted to squeeze the main changes in before the Tuesday's release. I kept two separate commits, but I can squash them if you prefer it.

The main changes are:

Rename the extension and all related modules from quarkus-panache-rest-* to quarkus-*-rest-data-panache
Move URL building logic (Path annotations and Location headers etc.) to one place outside of the method implementors.
Build controller path based on a resource rather than entity.
Make path configurable
Split @PanacheRestResource configuration annotation to two @ResourceProperties (to be used on an interface level and @OperationProperties (to be used on a method).

gytis · 2020-05-16T12:10:32Z

cc @geoand

geoand · 2020-05-16T13:53:06Z

Thanks, I'll take a look first thing Monday.

I assume you also want a review from @FroMage ?

gytis · 2020-05-16T15:45:01Z

Yeah that would be helpful.

geoand · 2020-05-17T04:53:09Z

Can you please rebase onto master to pick up a CI fix?

Thanks

...rest-data-panache/runtime/src/main/java/io/quarkus/rest/data/panache/ResourceProperties.java

geoand · 2020-05-18T05:21:52Z

...eployment/src/main/java/io/quarkus/rest/data/panache/deployment/PanacheCrudResourceInfo.java

 import io.quarkus.builder.item.MultiBuildItem;

-public final class PanacheCrudResourceBuildItem extends MultiBuildItem {
+public final class PanacheCrudResourceInfo extends MultiBuildItem {


Any reason why this was renamed?

Before it was already used by a processor which would extract separate bits of information and only pass what's needed to a specific method implementor. I made the method implementors more abstract now so processor wouldn't have to know too much details about them. Now it just passes this object to each implementor. So I assumed the name without "BuildItem" suffix would be more suitable. What do you think?

It just feels weird to have being a build item and not being called that. What we usually do in similar cases is to have a build item that just holds a reference to the regular class. Then in the non-build-step code you can just pass around the regular class. The build step classes would of course the build items to communicate.

Yeah that makes sense, I'll extract it to a separate class

geoand · 2020-05-18T05:39:39Z

...main/java/io/quarkus/rest/data/panache/deployment/properties/ResourcePropertiesAccessor.java

+            return classInfo.classAnnotation(RESOURCE_PROPERTIES_ANNOTATION);
+        }
+        if (classInfo.superName() != null) {
+            ClassInfo superControllerInterface = index.getClassByName(classInfo.superName());


Minor nipick (can be fixed later): The use of controller seems a little out of place. Perhaps use resource instead?

True. I'll take a note of this and will fix it separately after the release.

geoand · 2020-05-18T05:41:16Z

...che/deployment/src/main/java/io/quarkus/rest/data/panache/deployment/utils/ResourceName.java

+
+    private static final String[] SUFFIXES = { "controller", "resource" };
+
+    public static String fromClass(Class<?> resourceClass) {


This seems to be unused now, no?

True, I had a use for it initially. I'll get rid of it.

geoand · 2020-05-18T05:51:06Z

...rest-data-panache/runtime/src/main/java/io/quarkus/rest/data/panache/ResourceProperties.java

+     *
+     * Default: hyphenated resource name without a suffix. Ignored suffixes are `Controller` and `Resource`.
+     */
+    String path() default "";


Not that I think this will ever be an issue, but by using "" as the default value, you are effectively preventing users from making an panache rest data resource without a path. Now that's probably A good thing, but I am just bringing it up in case you hadn't considered it. In any case it would probably be safer to use some nonsensical default valur

Hmm, we could maybe change this later too. Adding a nonsense value and based on that not add an annotation at all. I need to add some default value because otherwise user will always have to specify a path when using this annotation.

Yeah, I understand the need for a default value, just saying that "" is not the proper one :)

Shall we change it separately after the release? I don't want to hold this PR for too long.

geoand

Seems fine to me!

gytis · 2020-05-18T08:38:14Z

@geoand I've added modifications as a separate commit. I have to either keep them like this or squash all three commits (including the rename). Otherwise squashing first and third commits causes a lot of conflicts, so will take a lot of time to go through them.

geoand · 2020-05-18T08:39:36Z

I think we can live with the extra commit :)

FroMage

Thanks for the quick work, and sorry about the late review. Aren't we missing the docs too?

...test/java/io/quarkus/hibernate/orm/rest/data/panache/deployment/CustomPathItemsResource.java

...est-data-panache/runtime/src/main/java/io/quarkus/rest/data/panache/OperationProperties.java

...rest-data-panache/runtime/src/main/java/io/quarkus/rest/data/panache/ResourceProperties.java

FroMage · 2020-05-18T16:37:59Z

OK, so we're only missing the docs, right?

gytis · 2020-05-18T16:47:03Z

I've just cherry-picked the docs commit from this PR #9254 so it would be easier for you to review and merge.

FroMage

OK, sorry I didn't realise we hadn't merged the docs PR yet. Thanks for bringing the commit over.

docs/src/main/asciidoc/rest-data-panache.adoc

gytis · 2020-05-18T21:07:06Z

Thanks for the review @FroMage. I've committed and squashed your suggestions.

gytis · 2020-05-19T07:11:56Z

OpenID test failed the CI. Shouldn't be related to this PR.

geoand · 2020-05-19T07:25:33Z

OpenID test failed the CI. Shouldn't be related to this PR.

Yeah, it's a known flaky test

FroMage

Great!

FroMage · 2020-05-19T07:37:12Z

I've restarted the CI tests

gytis · 2020-05-20T08:26:55Z

"Native Tests - Data3" is the one that runs native tests for this extension. It passed our tests and later failed the Reactive Pg Client. It's not related with this change

geoand · 2020-05-20T08:27:49Z

"Native Tests - Data3" is the one that runs native tests for this extension. It passed our tests and later failed the Reactive Pg Client. It's not related with this change

Yeah, thats the reason we added --fail-at-end :)

boring-cyborg bot added area/core area/dependencies Pull requests that update a dependency file area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure area/panache labels May 16, 2020

gytis force-pushed the panache-rest-paths branch from e53828c to eda585c Compare May 17, 2020 09:13

geoand reviewed May 18, 2020

View reviewed changes

...rest-data-panache/runtime/src/main/java/io/quarkus/rest/data/panache/ResourceProperties.java Show resolved Hide resolved

geoand reviewed May 18, 2020

View reviewed changes

gytis force-pushed the panache-rest-paths branch from eda585c to 3fc8b60 Compare May 18, 2020 08:34

geoand approved these changes May 18, 2020

View reviewed changes

gsmet added this to the 1.5.0 milestone May 18, 2020

gytis force-pushed the panache-rest-paths branch from 3fc8b60 to cd4ae3d Compare May 18, 2020 10:00

gytis mentioned this pull request May 18, 2020

Panache REST guide #9254

Closed

FroMage requested changes May 18, 2020

View reviewed changes

gytis force-pushed the panache-rest-paths branch from cd4ae3d to 6c2b63c Compare May 18, 2020 16:31

boring-cyborg bot added the area/documentation label May 18, 2020

FroMage requested changes May 18, 2020

View reviewed changes

gytis force-pushed the panache-rest-paths branch from 2d69196 to 9215d9e Compare May 18, 2020 21:06

FroMage approved these changes May 19, 2020

View reviewed changes

gsmet force-pushed the panache-rest-paths branch 4 times, most recently from 9339c07 to aac0081 Compare May 20, 2020 07:03

Gytis Trikleris added 3 commits May 20, 2020 10:42

Path config and Panache REST reorg

e34edcf

Rename REST data Panache extension

2e3e7df

REST data with Panache guide

deea0a5

gsmet force-pushed the panache-rest-paths branch from aac0081 to deea0a5 Compare May 20, 2020 08:43

gsmet merged commit 2de0459 into quarkusio:master May 20, 2020

gytis deleted the panache-rest-paths branch May 20, 2020 11:50


		private static final String[] SUFFIXES = { "controller", "resource" };

		public static String fromClass(Class<?> resourceClass) {

REST data Panache reorganisation #9373

REST data Panache reorganisation #9373

Uh oh!

Conversation

gytis commented May 16, 2020

Uh oh!

gytis commented May 16, 2020

Uh oh!

geoand commented May 16, 2020

Uh oh!

gytis commented May 16, 2020

Uh oh!

geoand commented May 17, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoand left a comment

Choose a reason for hiding this comment

Uh oh!

gytis commented May 18, 2020

Uh oh!

geoand commented May 18, 2020

Uh oh!

FroMage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FroMage commented May 18, 2020

Uh oh!

gytis commented May 18, 2020

Uh oh!

FroMage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gytis commented May 18, 2020

Uh oh!