PropertyKey abstraction
Hi all,
Looking at the `PropertyKey` abstraction in `3.x`, I was wondering
what advantages it brings over simple string constants. From my
perspective it has more cons than pros.
Pros:
* it is typesafe; a committer must create an instance of it to use it;
Cons:
* it is hard to list for documentation purposes. It would be easier
to document all available properties if they were declared as:
/**
* Makes Foo into bars.
*/
@Log4jProperty
private static final String FOO_BAR = "Foo.bar";
Such a definition can be trivially detected by an annotation
processor, whereas currently I would need to call `PropertyKey#getKey`
to even find the name of the property.
* the split into "component" and "name" isn't really used in real
code, so `PropertyKey` is basically a wrapper for a single String,
* IMHO the `getSystemKey` method does not belong in `PropertyKey`. It
is something that is used only by those property sources that are
global in the JVM (like env variables or system properties). Other
property sources like `TestPropertySource` have no use of it.
* since a `PropertyKey` is not a compile-time constant, we have
several `Constant` classes that contain string constants, so that we
can use them in tests (e.g. in the `@SetSystemProperty` annotation).
If you don't have any objections, I would prefer to refactor all these
`PropertyKey` implementations to simple classes that contain only
constant string fields.
In a **longer** term it would be nice to reintroduce type safety, but
for the **values** of properties instead of their keys. We could copy
the way Spring does it, create classes like:
/**
* Configures a https://logging.apache.org/log4j/3.x/recycler.html
*/
@Log4jPropertySource(prefix = "Recycler")
public class RecyclerConfig {
/**
* Type of recycler.
*/
public String type;
/**
* Size of the recyclers queue.
*/
public int size;
}
and we could retrieve all the properties at once with
`PropertyEnvironment#getProperties(Recycler.class)`.
What do you think?
Re: PropertyKey abstraction
PropertyKey was created so that all “components” could be specified as an enum
value, thus ensuring consistency.
The split between component and key is used in every declaration of a property.
It is unfortunate that the keys also had to be specified as static constants so
they could be used in @SetSystemProperty. I would have preferred to be able to
use the getSystemKey method.
I do have objections. While the interface and enum usage isn’t perfect moving
back to static constants is much worse in my opinion. If I hadn’t thought that
I would have done that in the first place.
If you can figure out a better approach just propose that but please leave it
the way it is until then.
I should note that some of the issues I encountered when I did this were:
1. Because PropertyCompoment is an enum all components have to be defined
globally even if they only apply to a specific module.
2. Each module needs to be able to define the properties that are specific to
that module.
3. The PropertyKey is actually the trailing portion of the key as it is defined
in the system. There they are always prefixed by the Log4j identifier and their
scope.
Ralph
> On Jan 4, 2024, at 2:13 AM, Piotr P. Karwasz wrote:
>
> Hi all,
>
> Looking at the `PropertyKey` abstraction in `3.x`, I was wondering
> what advantages it brings over simple string constants. From my
> perspective it has more cons than pros.
>
> Pros:
>
> * it is typesafe; a committer must create an instance of it to use it;
>
> Cons:
>
> * it is hard to list for documentation purposes. It would be easier
> to document all available properties if they were declared as:
>
> /**
> * Makes Foo into bars.
> */
> @Log4jProperty
> private static final String FOO_BAR = "Foo.bar";
>
> Such a definition can be trivially detected by an annotation
> processor, whereas currently I would need to call `PropertyKey#getKey`
> to even find the name of the property.
>
> * the split into "component" and "name" isn't really used in real
> code, so `PropertyKey` is basically a wrapper for a single String,
> * IMHO the `getSystemKey` method does not belong in `PropertyKey`. It
> is something that is used only by those property sources that are
> global in the JVM (like env variables or system properties). Other
> property sources like `TestPropertySource` have no use of it.
> * since a `PropertyKey` is not a compile-time constant, we have
> several `Constant` classes that contain string constants, so that we
> can use them in tests (e.g. in the `@SetSystemProperty` annotation).
>
> If you don't have any objections, I would prefer to refactor all these
> `PropertyKey` implementations to simple classes that contain only
> constant string fields.
>
> In a **longer** term it would be nice to reintroduce type safety, but
> for the **values** of properties instead of their keys. We could copy
> the way Spring does it, create classes like:
>
> /**
> * Configures a https://logging.apache.org/log4j/3.x/recycler.html
> */
> @Log4jPropertySource(prefix = "Recycler")
> public class RecyclerConfig {
>
> /**
> * Type of recycler.
> */
> public String type;
>
> /**
> * Size of the recyclers queue.
> */
> public int size;
> }
>
> and we could retrieve all the properties at once with
> `PropertyEnvironment#getProperties(Recycler.class)`.
>
> What do you think?
Re: PropertyKey abstraction
Hi Ralph, On Thu, 4 Jan 2024 at 16:20, Ralph Goers wrote: > > PropertyKey was created so that all “components” could be specified as an > enum value, thus ensuring consistency. > > The split between component and key is used in every declaration of a > property. > > It is unfortunate that the keys also had to be specified as static constants > so they could be used in @SetSystemProperty. I would have preferred to be > able to use the getSystemKey method. > > I do have objections. While the interface and enum usage isn’t perfect moving > back to static constants is much worse in my opinion. If I hadn’t thought > that I would have done that in the first place. > > If you can figure out a better approach just propose that but please leave it > the way it is until then. If consistency is the only requirement, we could use a static analysis tool to check it. It should be rather easy to write an Error Prone plugin that checks that: 1. The parameters of all calls to `PropertyEnvironment` methods are public constant strings annotated with a specific annotation (e.g. @Log4jPropertyKey), 2. The strings are of the format "." with a limited number of possibilities for . Remark that the list of components is not necessarily closed: additional Maven modules or even third-party extensions could need to define new component types. E.g. if we split `DisruptorBlockingQueueFactory` into a separate Maven module, it will still require an entry in `PropertyComponent` to have its own properties. Piotr
Re: PropertyKey abstraction
FWIW, I'm NOT a fan of the static analysis option. Good design should be reflected in the code. We should not pick suboptimal abstractions (or no abstraction in this case) plus static analysis to validate odd design choices. Gafy On Thu, Jan 4, 2024, 11:29 AM Piotr P. Karwasz wrote: > Hi Ralph, > > On Thu, 4 Jan 2024 at 16:20, Ralph Goers > wrote: > > > > PropertyKey was created so that all “components” could be specified as > an enum value, thus ensuring consistency. > > > > The split between component and key is used in every declaration of a > property. > > > > It is unfortunate that the keys also had to be specified as static > constants so they could be used in @SetSystemProperty. I would have > preferred to be able to use the getSystemKey method. > > > > I do have objections. While the interface and enum usage isn’t perfect > moving back to static constants is much worse in my opinion. If I hadn’t > thought that I would have done that in the first place. > > > > If you can figure out a better approach just propose that but please > leave it the way it is until then. > > If consistency is the only requirement, we could use a static analysis > tool to check it. It should be rather easy to write an Error Prone > plugin that checks that: > > 1. The parameters of all calls to `PropertyEnvironment` methods are > public constant strings annotated with a specific annotation (e.g. > @Log4jPropertyKey), > 2. The strings are of the format "." with a > limited number of possibilities for . > > Remark that the list of components is not necessarily closed: > additional Maven modules or even third-party extensions could need to > define new component types. E.g. if we split > `DisruptorBlockingQueueFactory` into a separate Maven module, it will > still require an entry in `PropertyComponent` to have its own > properties. > > Piotr >
