Injecting config into recorders

[Stuart Douglas]

Hi Everyone,

At the moment runtime config is not really handled very well. In order to pass it into a recorder it needs to first be passed through a @BuildStep @Record method. This is confusing as there is nothing valid that this build step can do with the object except pass it to a recorder, however we can't stop a build step from accidently trying to read a runtime value at build time.

This is problematic because runtime config can change, so build steps should never be looking at these values at build time. There are a few issues around this in the issue tracker (e.g. [1][2]), but we have just never got around to solving it.

To solve this I have implemented constructor injection into recorders for config objects [3]. Basically this means that you can avoid passing config into recorders from build steps altogether, and instead just use constructor injection and final fields.

One thing I am not sure about is if we should wait for the next release for this, given that this is supposed to be a stabilization release. I think it is ok as the changes will be extensively tested, however I would like to hear what others think.

Once this is in I can go through and update the extensions to use this as much as possible, but that has a high chance of conflict so I don't want to do it until it is in.

Stuart

[1] https://github.com/quarkusio/quarkus/issues/861

[2] https://github.com/quarkusio/quarkus/issues/5514

[3] https://github.com/quarkusio/quarkus/pull/18983

[Georgios Andrianakis]

Given the amount of testing this would get, I believe it's perfectly fine for it to go in

--
You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CAD%2BL2cxqNTKKQ6ZeoLvi_N1-77QJ33JkntLcfLe_AS49uQ7z0Q%40mail.gmail.com.

[Martin Kouba]

Just FTR - BUILD_AND_RUN_TIME_FIXED and RUN_TIME config roots can be
injected directly in any bean. But RUN_TIME config roots should only be
accessed at runtime, i.e. not during the STATIC_INIT bootstrap phase.

I do mention this here because very often, recorders just pass the
config info to some beans...

> <https://github.com/quarkusio/quarkus/pull/18983>
>
> --
> You received this message because you are subscribed to the Google
> Groups "Quarkus Development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to quarkus-dev...@googlegroups.com

> <mailto:quarkus-dev...@googlegroups.com>.

> To view this discussion on the web visit
> https://groups.google.com/d/msgid/quarkus-dev/CAD%2BL2cxqNTKKQ6ZeoLvi_N1-77QJ33JkntLcfLe_AS49uQ7z0Q%40mail.gmail.com

> <https://groups.google.com/d/msgid/quarkus-dev/CAD%2BL2cxqNTKKQ6ZeoLvi_N1-77QJ33JkntLcfLe_AS49uQ7z0Q%40mail.gmail.com?utm_medium=email&utm_source=footer>.

--
Martin Kouba
Software Engineer
Red Hat, Czech Republic

[Loïc MATHIEU]

Hi,

And what about BOOTSTAP config root ?
As I understand them, it's a special kind of RUNTIME evaluated very realy.

I had some challenges making them work, that's why I ask if they will obey the same rules.

Regards,

Loïc

To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/d44e252b-15bb-6661-a137-f898a3707d27%40redhat.com.

[Stuart Douglas]

On Mon, 26 Jul 2021 at 17:23, Loïc MATHIEU <loik...@gmail.com> wrote:

Hi,

And what about BOOTSTAP config root ?
As I understand them, it's a special kind of RUNTIME evaluated very realy.

I had some challenges making them work, that's why I ask if they will obey the same rules.

I think they should still work, I need to verify.

Something else I realised is that for STATIC_INIT recorders I need to make sure that any RUNTIME config is null, so you get an NPE if you use it wrong. It would be better if there was a better way or reporting this but I can't really think of any.

Stuart

[Georgios Andrianakis]

On Mon, Jul 26, 2021, 10:23 Loïc MATHIEU <loik...@gmail.com> wrote:

Hi,

And what about BOOTSTAP config root ?
As I understand them, it's a special kind of RUNTIME evaluated very realy.

I had some challenges making them work, that's why I ask if they will obey the same rules.

What sort of challenges are you referring to?

I am fairly certain this improvement will work with bootstrap config

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CAJLxjVHh9mPP-HgbJgCX4cd0WW%3DH1jUZ2qSq1o6safheckdbDA%40mail.gmail.com.

[Roberto Cortez]

I’m actually thinking in deprecating the BOOTSTRAP phase. We have a better way to initialize sources with some specific SR Config APIs, but it is not really high in the priority list, and definitely not before the stabilization release.

Cheers,

Roberto

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CALeTM-mE0Lu35CbNy2_Byczoeg%2BLBvmbZU08x4mtOjF4N8Uayw%40mail.gmail.com.

[Georgios Andrianakis]

On Mon, Jul 26, 2021 at 1:32 PM Roberto Cortez <radc...@yahoo.com> wrote:

I’m actually thinking in deprecating the BOOTSTRAP phase. We have a better way to initialize sources with some specific SR Config APIs, but it is not really high in the priority list, and definitely not before the stabilization release.

Yeah, we should look into this for 2.3 and beyond

[Stuart Douglas]

So I have been thinking about this a bit more, and I have come up with the following ideas about how to handle runtime config injections. As I see it there are a few options:

1) Have runtime config be null during build time. I don't really like this, I think it is a bit user hostile and non-obvious. 

2) Inject something like RuntimeValue<MyConfig> for runtime config. This would throw an exception if you try to use it at build time. 

3) Separate recorders into runtime recorders and static init recorders, and only allow injection into the appropriate recorder types

I think 2) or 3) is the way to go, and I am leaning towards 2) as it is similar to what we already have.

Unless anyone objects I am going to update the PR to use approach 2).

Stuart

[Guillaume Smet]

I think it's a very good thing to have as a lot of extension developers got bitten by it but I don't think it should go in 2.2.

While it might not introduce bugs, it also doesn't fix any AFAICT so the ratio benefits/risks is not good enough.

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CAD%2BL2czx5iZytHLD_Etv970U%3D_g9sb%2B0yPJQqhEbn1nXbA2gMQ%40mail.gmail.com.

[Stuart Douglas]

On Mon, 16 Aug 2021 at 16:59, Guillaume Smet <guillau...@gmail.com> wrote:

I think it's a very good thing to have as a lot of extension developers got bitten by it but I don't think it should go in 2.2.

While it might not introduce bugs, it also doesn't fix any AFAICT so the ratio benefits/risks is not good enough.

Yes, it's gotten too late in the cycle, it should definitely be a 2.3 thing.

Stuart

[Alexey Loubyansky]

I'd go with 2 as well.

On Mon, Aug 16, 2021 at 3:16 AM Stuart Douglas <sdou...@redhat.com> wrote:

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CAD%2BL2czx5iZytHLD_Etv970U%3D_g9sb%2B0yPJQqhEbn1nXbA2gMQ%40mail.gmail.com.

[Stuart Douglas]

I have finally got around to revisiting this, and it should now be good to go (assuming CI passes). Basically you can inject any config using constructor injection, however if the recorder is used for both runtime and static init methods, and injects runtime config, then you need to wrap it in a RuntimeValue, e.g. [1].

Stuart

[1] https://github.com/quarkusio/quarkus/pull/18983/files#diff-902af2bee29f3da057d18e68c0836c03bcaeeff5e40e17524801c1261af1bc2fR159

[Knut Wannheden]

Does this then also mean that there will now be an error or warning logged when a build step receives a runtime config object as input? I think that would be nice to avoid accidental bugs.

Knut

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CAD%2BL2cyMFwSBt1VUX%2Bmc9Yq%3DeSsEUGhdSPn5qemZK_2NUohkRQ%40mail.gmail.com.

[Stuart Douglas]

Not just yet. For now both approaches are supported as there will be a lot of existing code that relies on the old approach.

Once this has been in for a while we can deprecate the existing approach and start to warn about it.

Stuart