Oshi: PerfCounterQuery constructor does not properly test propertyEnum argument

[Haselhuhn]

I was planning on working on #803, but I got distracted and @yehia67 beat me to it.

However, unless I'm mistaken, the portion of code addressed by #803 doesn't seem to be correct. An exception is being thrown if the class being checked IS a subclass of PdhCounterProperty, which seems to be the opposite of the intent.

Also, the class passed to isAssignableFrom is the class containing the "property enum" (which, as you noted, is null if the enum is a top-level class) instead of the enum's class itself. Instead, I believe it should be the following:

if (!PdhCounterProperty.class.isAssignableFrom(propertyEnum)) {

This causes another problem as the PerfCounterWildcardQuery constructor also invokes the PdhCounterProperty constructor. The end result is that a property enum passed to PerfCounterWildcardQuery would be required to implement both PdhCounterProperty and PdhCounterWildcardProperty.

[dbwiddis]

Wow, good catch. Yes, the test is clearly missing the ! but fortunately(?) the test is wrong so the exception is never thrown!

A proper junit test would have caught this, of course. ;)

Can you submit the fix?

[dbwiddis]

I think the conditional should be either:
if(!PdhCounterProperty.class.isInstance(propertyEnum)) {
or
if(!PdhCounterProperty.class.isAssignableFrom(propertyEnum.getClass())) {

Do you agree?

[dbwiddis]

Also there should be a similar check on the Wildcard Query constructor in addition to the one that will occur after super().

[Haselhuhn]

I'm looking into it. Glad to help.

[Haselhuhn]

It would not be possible to do the similar check on the PerfCounterWildcardQuery constructor, since it also calls the PerfCounterQuery constructor. The propertyEnum argument is not implementing both PdhCounterProperty and PdhCounterWildcardProperty.

Is there a need for both *Property interfaces? I haven't been able to find one.

Do you think it would be fine to return the instance filter for wildcard properties from PdhCounterProperty.getInstance()? Then we could fix the propertyEnum argument test in PdhCounterQuery alone.

[dbwiddis]

I did think about having just a single Property type, but there are quire a few differences beyond just the instance. Fundamentally, one returns a single counter value, while the other returns a list of 0 or more values. For example, the PhysicalDisk counter needs the wildcard. I want all counters of the form PhysicalDisk(*)\Disk Reads/sec, which will return counters for PhysicalDisk(0 C:)\Disk Reads/sec, and PhysicalDisk(1 D:)\Disk Reads/sec. In this case I don't know how many I'm going to get and the order isn't guaranteed so I need to get the instances (0 C: and 1 D:). In contrast, for the swap file there's only one counter for Memory\Pages Input/sec, with no associated instance.

Now back to the checks. For the non-wildcard query, the Property Enum has to implement PerfCounterQuery. We can check that in the constructor of PerfCounterQuery that the enum argument is an instance of this interface, as described above.

For the wildcard query, the Property Enum has to implement PerfCounterWildcardQuery which itself extends PerfCounterQuery (thus the Enum implements both interfaces). We can can check that in the constructor of PerfCounterWildCardQuery that the enum argument is an instance of PerfCounterWildcardQuery. And since it calls super() that will independently also check that the enum argument is also an instance of PerfCounterQuery.

So my suggestion is in the constructor of PerfCounterQuery, throw an exception
if(!PdhCounterProperty.class.isInstance(propertyEnum))

And in the constructor of PerfCounterWildcardQuery first call super() which does the above check, and then also throw an exception
if(!PdhCounterWildcardProperty.class.isInstance(propertyEnum))

[Haselhuhn]

The property enums implement either PdhCounterPropertyor PdhCounterWildcardPropertyand are then passed to PerfCounterQuery and PerfCounterWildcardQuery.

We could create a new protected constructor in PerfCounterQuery that didn't perform the check for PdhCounterProperty, but that's not an elegant solution. It feels like PdhCounterWildcardProperty does not have enough in common, as implemented, to be a subclass of PdhCounterProperty.

[dbwiddis]

Ah, you're right I was forgetting which things inherited which. I was thinking that we'd done this (which we haven't but could):

    public interface PdhCounterProperty extends PdhCounterWildcardProperty {
        /**
         * @return Returns the instance.
         */
        String getInstance();
    }

However, after doing that and trying to implement my suggestion, I realized what I'm trying to do just can't be done. Because the method argument is cast to Class, that's all we can get out of the argument. What we _really_ want to test is the generic type T but that can't be done at runtime.

So, really, the correct solution is just to completely remove that test. It's (fortunately) wrong now and does nothing, but should be completely removed as it's impossible to do what's intended. I think we have compile-time checks on the fact that if the enum is wrong they can't use the getInstance() or getCounter() methods anyway, so there is no runtime check needed.

[Haselhuhn]

I agree. I still think there is a better design that would have better code reuse and less duplication, but for now I'm just going to remove the check in the constructor.

I'll file another issue if I come up with an alternative design.

[dbwiddis]

One possible design is just to have only the "wildcard" version, and then have a few util getter methods for the "single instance" (usually "_Total") / "null instance" versions that bypass the lists and just return the first element. A little extra overhead to create a List when we know we're only getting one element, but if it improves readability/usability/testability I wouldn't mind that.

[Haselhuhn]

That is an option. I have some other ideas as well, that may or may not be good.

For now, I'll go ahead and remove the ineffective check, and perhaps start another issue for these design discussions of PerfCounterQuery.