Support for uri/uriabs from Quarkus Renarde #586

angelozerr · 2022-03-08T15:15:10Z

Support for uri/uriabs from Quarkus Renarde

Fixes #571

This PR uses those renarde validation rules:

all parameters which are annotated with @RestPath (or @PathParam) are considered as required
all parameters which are annotated with @restquery (or @QueryParam) are optional
all parameters which are annotated with @restform (or @FormParam) cannot appear in the method of uri.

Signed-off-by: azerr azerr@redhat.com

angelozerr · 2022-03-08T15:19:23Z

Here a little demo with renarde uri:

//c @FroMage

FroMage · 2022-03-08T15:32:54Z

Wow, super nice!

angelozerr · 2022-03-08T16:26:12Z

There are 2 problems with diagnostics in quarkus-renarde-todo in Login.html:

Problem 1:

It's because we check that allowed method doesn't return void according to this rule https://github.com/quarkusio/quarkus/blob/ce19ff75e9f732ff731bb30c2141b44b42c66050/independent-projects/qute/core/src/main/java/io/quarkus/qute/ReflectionValueResolver.java#L176

@mkouba why ReflectionValueResolver forbids the void although renarde value resolver doesn't forbid it? If we want to keep this behavior,it means that we must provide custom validation for renarde.

Problem 2:

It's because manualLogin expect 2 parameters:

    @POST
    public Response manualLogin(@NotBlank @RestForm String userName, 
            @NotBlank @RestForm String password) {

It seems that userName and password comes from #formElement

How to manage this rule with generic mean?

@FroMage @mkouba have you some idea?

FroMage · 2022-03-08T16:33:36Z

@mkouba why ReflectionValueResolver forbids the void although renarde value resolver doesn't forbid it? If we want to keep this behavior,it means that we must provide custom validation for renarde.

Actually this doesn't go through ReflectionValueResolver, because custom namespaces can do whatever they want.

How to manage this rule with generic mean?

Well, this is because the method only requires parameters that end up in the URI, such as @PathParam, but all the others can be ignored when calling uri (those that are context, or headers, cookies, or form params in this case).

So yeah, namespace resolvers are really like primitives and must be hard-coded: you can't guess what they do.

mkouba · 2022-03-08T16:39:57Z

@mkouba why ReflectionValueResolver forbids the void although renarde value resolver doesn't forbid it? If we want to keep this behavior,it means that we must provide custom validation for renarde.

Actually this doesn't go through ReflectionValueResolver, because custom namespaces can do whatever they want.

Stef is correct that we only validate output expressions where a namespace is mapped to a @TemplateExtension method or @TemplateData with a namespace. Otherwise there is no type info available. In this case, I assume that uri:RenardeSecurityController.loginUsingOidc does not invoke the controller method at all.

FroMage · 2022-03-08T16:45:52Z

Indeed. It's only used to invoke the reverse-router.

angelozerr · 2022-03-08T19:39:13Z

Thanks @FroMage @mkouba for our feedback.

It wil require to support custom validation for a given namespace.

I will do that in an another issues. For the moment we will have those errors.

mkouba · 2022-03-09T07:56:10Z

It wil require to support custom validation for a given namespace.

In general, an output expression that starts with a namespace should only be validated (java model) if the namespace is inject, cdi or corresponds to a @TemplateEnum, @TemplateData#namespace() and a namespace extension method. All other namespaces should be ignored.

angelozerr · 2022-03-09T08:37:50Z

All other namespaces should be ignored.

I understand the idea, but my idea is to provide the capability to provide custom validation like renarde validation.

fbricon · 2022-03-09T11:16:57Z

So if I'm using the uri: or uriabs: namespace on a non-renarde project (renarde jars not on the project's classpath), the tool behaves as if the resolver was known and available, as opposed to actually complaining about an unknown namespace.

angelozerr · 2022-03-09T13:05:23Z

So if I'm using the uri: or uriabs: namespace on a non-renarde project (renarde jars not on the project's classpath), the tool behaves as if the resolver was known and available, as opposed to actually complaining about an unknown namespace.

Good catch! It should be fixed now.

fbricon · 2022-03-09T18:03:23Z

In https://github.com/FroMage/quarkus-renarde-todo/blob/main/src/main/resources/templates/Todos/index.html#L24, the target Controller is resolved:

angelozerr · 2022-03-09T18:48:13Z

...c/main/java/com/redhat/qute/jdt/internal/extensions/renarde/UriNamespaceResolverSupport.java

+			try {
+				// Find all classes which extends 'io.quarkiverse.renarde.Controller'
+				ITypeHierarchy typeHierarchy = type.newTypeHierarchy(monitor);
+				IType[] controllerTypes = typeHierarchy.getAllClasses();


@rgrunber here I need to collect all classes which extends "io.quarkiverse.renarde.Controller" class. I have a usecase with :

Application which extends Controller

Todos which extends ControllerWithUser which extends Controller.

When Controller and ControllerWithUser comes from sources (I copied/pasted thoses sources in my project), I can get Application and Todos.

But when Controller and ControllerWithUser comes from JAR,it seems (according the problem with @fbricon with Todos), that it get only Application (notTodos which extends ControllerWithUser which extends Controller).

At first do you think my code is correct? Have you some idea about this problem?

Is it the same result if you try using IType[] getAllSubtypes(IType type); from ITypeHierarchy ?

The first thing I notice from running the test case is that

/qute-renarde/lib/quarkus-renarde-1.0.0-SNAPSHOT.jar[CPE_LIBRARY][K_BINARY][isExported:false][attributes:module=true] /qute-renarde/lib/quarkus-renarde-oidc-1.0.0-SNAPSHOT.jar[CPE_LIBRARY][K_BINARY][isExported:false][attributes:module=true]

don't exist. They're defined in the .classpath so they show, but there's no lib folder in the qute-renard project so no way to even get the type. I assume you may have them locally so I think they probably need to be added to the commit.

Update : In fact, If I run javaProject.getResource().findMarkers(null, true, org.eclipse.core.resources.IResource.DEPTH_INFINITE), I get :

Marker [on: /qute-renarde, id: 5, type: org.eclipse.jdt.core.buildpath_problem, attributes: [arguments: 0:, categoryId: 10, classpathFileFormat: false, cycleDetected: false, id: 964, location: Build path, message: Project 'qute-renarde' is missing required library: 'lib/quarkus-renarde-oidc-1.0.0-SNAPSHOT.jar', outputOverlappingSource: false, severity: 2, sourceId: JDT], created: 3/10/22, 4:15 PM] Marker [on: /qute-renarde, id: 4, type: org.eclipse.jdt.core.buildpath_problem, attributes: [arguments: 0:, categoryId: 10, classpathFileFormat: false, cycleDetected: false, id: 964, location: Build path, message: Project 'qute-renarde' is missing required library: 'lib/quarkus-renarde-1.0.0-SNAPSHOT.jar', outputOverlappingSource: false, severity: 2, sourceId: JDT], created: 3/10/22, 4:15 PM]

...c/main/java/com/redhat/qute/jdt/internal/extensions/renarde/UriNamespaceResolverSupport.java

angelozerr · 2023-01-31T05:48:55Z

Okay. Makes sense. Be sure to leave a comment or open an issue for that.

See #780

datho7561 · 2023-01-31T15:42:51Z

If I have a method, and I "pass" it more parameters than it has, then there is no error. I don't know if this is intentional

@angelozerr do you want to handle this in this PR or should I open an issue for it?

angelozerr · 2023-01-31T15:49:34Z

@angelozerr do you want to handle this in this PR or should I open an issue for it?

I think it is a bug with space. If you write

<a href={uri:Login.foo(null,"1234")}>click me</a>

it should work, no?

I would like to do that in a separate PR since it is not linked to this PR

angelozerr · 2023-01-31T15:50:21Z

@datho7561 I need to write JDT test to collect ResolvedJavaTypeInfo for renarde.

datho7561 · 2023-01-31T15:59:04Z

I think it is a bug with space.

You're right, I can confirm it works if I don't add spaces.

The qute-ls server is not shutting down properly, but I think that is also not related to this PR.

angelozerr · 2023-01-31T16:34:33Z

@datho7561 I need to write JDT test to collect ResolvedJavaTypeInfo for renarde.

done quickly

angelozerr · 2023-01-31T16:50:35Z

The qute-ls server is not shutting down properly, but I think that is also not related to this PR.

No, but it is not related with new vscode language client?

datho7561 · 2023-01-31T17:15:36Z

I did some testing, and it looks like you were right: it's related to the new language client.

@restquery

Fixes redhat-developer#571 This PR uses those renarde validation rules: * all parameters which are annotated with @RestPath (or @PathParam) are considered as required * all parameters which are annotated with @restquery (or @QueryParam) are optional * all parameters which are annotated with @restform (or @FormParam) cannot appear in the method of uri. Signed-off-by: azerr <azerr@redhat.com>

angelozerr · 2023-01-31T17:48:52Z

I did some testing, and it looks like you were right: it's related to the new language client.

I removed https://github.com/redhat-developer/vscode-quarkus/pull/506/files#diff-c1b7030f8c5f91b6d8690295e037104c11aecfc6d7eb7a9bd531aa0a9c5547fdL40 because I though this language client fix the problem. If you add again shouldLanguageServerExitOnShutdown: true is shutdown working better?

If it works better,it mean that we have the same problem with vscode-microprofile and vscode-xml -(

angelozerr · 2023-01-31T17:53:30Z

@datho7561 tests pass now, if you think it is good, please merge it.

datho7561

Looks good. Thanks, Angelo!

angelozerr · 2023-01-31T19:53:57Z

Thanks so much @datho7561 for your great review!

@FroMage to have been patient and give us great feedback. This PR is à good start for supporting renarde but we need to improve again the support.

@FroMage when you will have time please install vscode quarkus prerelease when it will be available. I think tomorrow prerelease should be available.

Please install too vscode microprofile prerelease to avoid having problem with quarkus support

openshift-ci bot added the do-not-merge/work-in-progress label Mar 8, 2022

angelozerr force-pushed the uri-renarde branch 2 times, most recently from c9bd18b to ac04ded Compare March 8, 2022 16:15

angelozerr force-pushed the uri-renarde branch 2 times, most recently from 7511b79 to 6076d7f Compare March 9, 2022 08:28

angelozerr marked this pull request as ready for review March 9, 2022 08:28

openshift-ci bot removed the do-not-merge/work-in-progress label Mar 9, 2022

angelozerr force-pushed the uri-renarde branch from 6076d7f to 5df91e0 Compare March 9, 2022 08:33

angelozerr requested a review from fbricon March 9, 2022 08:38

angelozerr force-pushed the uri-renarde branch from 5df91e0 to e68cefd Compare March 9, 2022 10:59

angelozerr force-pushed the uri-renarde branch from e68cefd to ec58f50 Compare March 9, 2022 13:00

angelozerr force-pushed the uri-renarde branch from ec58f50 to 7955c95 Compare March 9, 2022 15:46

angelozerr commented Mar 9, 2022

View reviewed changes

angelozerr force-pushed the uri-renarde branch 5 times, most recently from 5ffd259 to 5925a6c Compare March 10, 2022 13:01

rgrunber reviewed Jan 30, 2023

View reviewed changes

...c/main/java/com/redhat/qute/jdt/internal/extensions/renarde/UriNamespaceResolverSupport.java Outdated Show resolved Hide resolved

angelozerr force-pushed the uri-renarde branch 3 times, most recently from 3856c01 to dee17c2 Compare January 31, 2023 05:43

angelozerr mentioned this pull request Jan 31, 2023

Improve validation of method parameters in renarde uri/uriabs #780

Open

angelozerr force-pushed the uri-renarde branch 2 times, most recently from 3dc2cac to 8725b66 Compare January 31, 2023 16:33

angelozerr force-pushed the uri-renarde branch 3 times, most recently from 09558c9 to 24b7dea Compare January 31, 2023 16:49

angelozerr force-pushed the uri-renarde branch 5 times, most recently from c1c3c3c to 6885ba3 Compare January 31, 2023 17:03

angelozerr force-pushed the uri-renarde branch from 6885ba3 to fd52efc Compare January 31, 2023 17:45

datho7561 approved these changes Jan 31, 2023

View reviewed changes

datho7561 merged commit 05e23f9 into redhat-developer:master Jan 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for uri/uriabs from Quarkus Renarde #586

Support for uri/uriabs from Quarkus Renarde #586

angelozerr commented Mar 8, 2022 •

edited

Loading

angelozerr commented Mar 8, 2022

FroMage commented Mar 8, 2022

angelozerr commented Mar 8, 2022 •

edited

Loading

FroMage commented Mar 8, 2022

mkouba commented Mar 8, 2022 •

edited

Loading

FroMage commented Mar 8, 2022

angelozerr commented Mar 8, 2022

mkouba commented Mar 9, 2022

angelozerr commented Mar 9, 2022

fbricon commented Mar 9, 2022

angelozerr commented Mar 9, 2022

fbricon commented Mar 9, 2022

angelozerr Mar 9, 2022

rgrunber Mar 10, 2022 •

edited

Loading

rgrunber Mar 10, 2022 •

edited

Loading

angelozerr commented Jan 31, 2023

datho7561 commented Jan 31, 2023

angelozerr commented Jan 31, 2023

angelozerr commented Jan 31, 2023

datho7561 commented Jan 31, 2023 •

edited

Loading

angelozerr commented Jan 31, 2023

angelozerr commented Jan 31, 2023

datho7561 commented Jan 31, 2023

angelozerr commented Jan 31, 2023

angelozerr commented Jan 31, 2023

datho7561 left a comment

angelozerr commented Jan 31, 2023

Support for uri/uriabs from Quarkus Renarde #586

Support for uri/uriabs from Quarkus Renarde #586

Conversation

angelozerr commented Mar 8, 2022 • edited Loading

angelozerr commented Mar 8, 2022

FroMage commented Mar 8, 2022

angelozerr commented Mar 8, 2022 • edited Loading

FroMage commented Mar 8, 2022

mkouba commented Mar 8, 2022 • edited Loading

FroMage commented Mar 8, 2022

angelozerr commented Mar 8, 2022

mkouba commented Mar 9, 2022

angelozerr commented Mar 9, 2022

fbricon commented Mar 9, 2022

angelozerr commented Mar 9, 2022

fbricon commented Mar 9, 2022

angelozerr Mar 9, 2022

Choose a reason for hiding this comment

rgrunber Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

rgrunber Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

angelozerr commented Jan 31, 2023

datho7561 commented Jan 31, 2023

angelozerr commented Jan 31, 2023

angelozerr commented Jan 31, 2023

datho7561 commented Jan 31, 2023 • edited Loading

angelozerr commented Jan 31, 2023

angelozerr commented Jan 31, 2023

datho7561 commented Jan 31, 2023

angelozerr commented Jan 31, 2023

angelozerr commented Jan 31, 2023

datho7561 left a comment

Choose a reason for hiding this comment

angelozerr commented Jan 31, 2023

angelozerr commented Mar 8, 2022 •

edited

Loading

angelozerr commented Mar 8, 2022 •

edited

Loading

mkouba commented Mar 8, 2022 •

edited

Loading

rgrunber Mar 10, 2022 •

edited

Loading

rgrunber Mar 10, 2022 •

edited

Loading

datho7561 commented Jan 31, 2023 •

edited

Loading