redhat-developer / quarkus-ls

Language server for Quarkus tooling
Eclipse Public License 2.0
44 stars 15 forks source link

inject:csrf is not retrieved in Qute template #901

Closed angelozerr closed 1 day ago

angelozerr commented 11 months ago

In https://github.com/ia3andy/quarkus-blast/blob/aa969f34616ec63be0268fee20a40097d2ec1ba4/src/main/resources/templates/layout/page.html#L11 the crsfis marked as Undefined.

This inject is defined in https://github.com/quarkusio/quarkus/blob/4f843d57f97048fc5fc499dab2ba1fdd1549d48d/extensions/csrf-reactive/runtime/src/main/java/io/quarkus/csrf/reactive/runtime/CsrfTokenParameterProvider.java#L13

I think the problems comes from our isValidBean method It is because https://github.com/redhat-developer/quarkus-ls/blob/dd9791a7c6ea74ca7ac7ef95ec69d9b6acee32cf/qute.jdt/com.redhat.qute.jdt/src/main/java/com/redhat/qute/jdt/utils/CDIUtils.java#L77

which should respect the spec https://jakarta.ee/specifications/cdi/3.0/jakarta-cdi-spec-3.0#what_classes_are_beans (the only thing that we don't support is the @Inject inconstructor parameter).

But when I read https://jakarta.ee/specifications/cdi/3.0/jakarta-cdi-spec-3.0#what_classes_are_beans the https://github.com/quarkusio/quarkus/blob/4f843d57f97048fc5fc499dab2ba1fdd1549d48d/extensions/csrf-reactive/runtime/src/main/java/io/quarkus/csrf/reactive/runtime/CsrfTokenParameterProvider.java#L13 have a construcor parameter (it should not be a proper bean). Perhaps it misses Inject for the construr parameter?

@ia3andy have you some idea?

angelozerr commented 11 months ago

@FroMage have you some idea too?

FroMage commented 11 months ago

Oh, that's a CDI question, ask @mkouba :)

angelozerr commented 11 months ago

Ok thanks @FroMage for your answer, I'm waiting for an answer from @mkouba

mkouba commented 11 months ago

But when I read https://jakarta.ee/specifications/cdi/3.0/jakarta-cdi-spec-3.0#what_classes_are_beans the https://github.com/quarkusio/quarkus/blob/4f843d57f97048fc5fc499dab2ba1fdd1549d48d/extensions/csrf-reactive/runtime/src/main/java/io/quarkus/csrf/reactive/runtime/CsrfTokenParameterProvider.java#L13 have a construcor parameter (it should not be a proper bean). Perhaps it misses Inject for the construr parameter?

In Quarkus, we don't require @Inject to be declared on a single constructor. See https://quarkus.io/guides/cdi-reference#simplified-constructor-injection.

I assume that you're trying to validate that a bean with the name crsf exists. But I'm afraid that you won't be able to perform the validation correctly. Mainly because not all beans are backed by a Java class (https://quarkus.io/guides/cdi-integration#synthetic_beans). Also note that producer beans can declare the name too.

fbricon commented 11 months ago

@mkouba could we imagine quarkus (deployment) jars embed some metadata files describing beans available at runtime?

fbricon commented 11 months ago

@angelozerr we'll probably need to hardcode such metadata in quarkus-ls for a bit

mkouba commented 11 months ago

@mkouba could we imagine quarkus (deployment) jars embed some metadata files describing beans available at runtime?

I don't think we would be able to automate the generation of such metadata so it wouldn't be very convenient from the extension author's point of view :shrug:.

ia3andy commented 11 months ago

@mkouba could we imagine quarkus (deployment) jars embed some metadata files describing beans available at runtime?

I don't think we would be able to automate the generation of such metadata so it wouldn't be very convenient from the extension author's point of view 🤷.

We must have something similar to generate the native data, no?

cc @geoand

geoand commented 11 months ago

Yes, we do generate a JSON file for the native stuff.

@mkouba why would generating a metadata file be a problem?

mkouba commented 11 months ago

could we imagine quarkus (deployment) jars embed some metadata files describing beans available at runtime?

We must have something similar to generate the native data, no?

Yes, we do generate a JSON file for the native stuff.

@mkouba why would generating a metadata file be a problem?

First of all, this is something different. We do generate for example reflect-config.json for the native image during application build. But this would need to be part of the -deployment jars of an extension, i.e. generated by the maven plugin. So the maven plugin would need to collect all information without executing build steps. Which is not a trivial task because we don't know which classes will be registered as additional beans, which synthetic beans are created, etc. I think that the only feasible way would be to instruct the extension authors to write the metadata manually. Which is IMO extremely inconvenient, error-prone and difficult to maintain.

ia3andy commented 11 months ago

@mkouba can't we generate it as part of the target app?

geoand commented 11 months ago

I don't see why we are talking about the deployment module and not the target application.

mkouba commented 11 months ago

@mkouba can't we generate it as part of the target app?

But then you wouldn't be able to use it from the IDE plugin, or?

mkouba commented 11 months ago

I don't see why we are talking about the deployment module and not the target application.

That was the original question from @fbricon.

geoand commented 11 months ago

Why not?

Couldn't the IDE plugin just work if the application is built? Sure it's not great, but it's better than nothing

mkouba commented 11 months ago

Why not?

Couldn't the IDE plugin just work if the application is built? Sure it's not great, but it's better than nothing

I don't think it's a good idea. I can imagine all kinds of synchronization issues (version upgrade, add/remove extension, mvn clean from the command line, etc.).

ia3andy commented 11 months ago

Why not? Couldn't the IDE plugin just work if the application is built? Sure it's not great, but it's better than nothing

I don't think it's a good idea. I can imagine all kinds of synchronization issues (version upgrade, add/remove extension, mvn clean from the command line, etc.).

it's the same problem for all built files no? even application.properties get synced on built

geoand commented 11 months ago

For sure. It's not great.

But for the record, I do agree that having the metadata file in the deployment of each extension is a no go

geoand commented 11 months ago

An alternative idea would be for the plugin to work with a running dev mode application - that is use an endpoint to get the metadata

On Wed, Aug 9, 2023, 12:43 Georgios Andrianakis @.***> wrote:

On Wed, Aug 9, 2023, 12:09 Martin Kouba @.***> wrote:

Why not?

Couldn't the IDE plugin just work if the application is built? Sure it's not great, but it's better than nothing

I don't think it's a good idea. I can imagine all kinds of synchronization issues (version upgrade, add/remove extension, mvn clean from the command line, etc.).

For sure. It's not great.

But for the record, I do agree that having the metadata file in the deployment of each extension is a no go

— Reply to this email directly, view it on GitHub https://github.com/redhat-developer/quarkus-ls/issues/901#issuecomment-1670955527, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBMDP75KISWMSVA5NA2ZVLXUNHUDANCNFSM6AAAAAA2QOO66U . You are receiving this because you were mentioned.Message ID: @.***>

mkouba commented 11 months ago

An alternative idea would be for the plugin to work with a running dev mode application - that is use an endpoint to get the metadata On Wed, Aug 9, 2023, 12:43 Georgios Andrianakis @.> wrote: On Wed, Aug 9, 2023, 12:09 Martin Kouba @.> wrote: > Why not? > > Couldn't the IDE plugin just work if the application is built? Sure it's > not great, but it's better than nothing > > I don't think it's a good idea. I can imagine all kinds of > synchronization issues (version upgrade, add/remove extension, mvn clean > from the command line, etc.). > For sure. It's not great. But for the record, I do agree that having the metadata file in the deployment of each extension is a no go > — > Reply to this email directly, view it on GitHub > <#901 (comment)>, > or unsubscribe > https://github.com/notifications/unsubscribe-auth/ABBMDP75KISWMSVA5NA2ZVLXUNHUDANCNFSM6AAAAAA2QOO66U > . > You are receiving this because you were mentioned.Message ID: > @.***> >

Good point. The JSON returned from GET http://localhost:8080/q/arc/beans should contain all relevant info, i.e. the @Named qualifier with value, bean class, etc.

fbricon commented 11 months ago

that would require the users to launch their app so we get proper completion inside the IDE. -1 from me

geoand commented 11 months ago

As mentioned, the metadata file in the deployment module is totally not doable

fbricon commented 11 months ago

to be more clear: launching the app to get additional info would be fine (springboot does it), but having broken validation until its launched, nope

geoand commented 11 months ago

Your plugin, your decision.

But as mentioned, the metadata idea is not possible in Quarkus

ia3andy commented 11 months ago

@fbricon what about having a soft validation (without errors when we don't know) when the metadata file is not there yet and have a stronger validation when it's there (after the first build)

fbricon commented 11 months ago

quarkus-ls has a mechanism to allow ignoring unknown properties, but we need to adopt it in intellij-quarkus (requires #794, #811 and #1043). we can also probably hard-code a bunch of known values in quarkus-ls (meh) but it'll work, should be simpler to do than anything else

angelozerr commented 11 months ago

Recognize inject:csrf is not only helpfull for validation,it is too for completion, hyperlink, hover, etc

So I think we should hard code that to benefit with those features

ia3andy commented 11 months ago

honestly we don't have a lot of extensions providing Qute stuff, having an api to generate a metadata file in the deployment to describe what the extension does for Qute might be a good idea?

angelozerr commented 11 months ago

For our tooling, it should be very welcome! If we can avoid hard coding something it would be really fantastic and avoid maintain that. We just read metadata from JARS and it will be sure that we will provide the proper data according to the version of the JAR used in the project.

We could specify this metadata file and if an author of the library want to have a good support in IDE/editor it will provide this metadata file.

geoand commented 11 months ago

That likely won't work because extensions do various things in their code and in a lot of cases the only way to get the metadata you need is to have the build steps executed. So I don't think you can expect that to happen.

ia3andy commented 11 months ago

@geoand it could be done by the quarkus-extension-processor and some way to indicate what is provided by the extensions

ia3andy commented 11 months ago

I don't mean run the @BuildStep, I mean some other way

mkouba commented 11 months ago

honestly we don't have a lot of extensions providing Qute stuff, having an api to generate a metadata file in the deployment to describe what the extension does for Qute might be a good idea?

Keep in mind that this issue is not about Qute stuff but CDI @Named beans...

geoand commented 11 months ago

No it can't.

As I said, all the intelligence is in the build steps, and unless an extension only does static things, there is no way around running the build steps. That said, it is possible to do a custom run without the application running - you can basically request certain build items. It's not trivial to do, but it's possible

FroMage commented 11 months ago

In https://github.com/quarkusio/quarkus/issues/13593#issuecomment-738065892 we wanted to know the set of CDI bean types available so we could know whether a JAX-RS endpoint parameter needed to be injected via CDI or not.

Here, we're looking for their names, but it sounds like a similar issue, no?

mkouba commented 11 months ago

In quarkusio/quarkus#13593 (comment) we wanted to know the set of CDI bean types available so we could know whether a JAX-RS endpoint parameter needed to be injected via CDI or not.

Here, we're looking for their names, but it sounds like a similar issue, no?

Not really. IIANM in quarkusio/quarkus#13593 you wanted to know the types from within a build step. Here we're talking about the IDE, no build steps.

angelozerr commented 1 day ago

Fixed with https://github.com/redhat-developer/quarkus-ls/pull/964