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 <piotr.karw...@gmail.com> 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?
