Bug 60381 - Inconsistent toString() in ValveBase and RealmBase
Summary: Inconsistent toString() in ValveBase and RealmBase
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.5.x-trunk
Hardware: All All
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-16 12:38 UTC by Michael Osipov
Modified: 2016-11-25 09:49 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Osipov 2016-11-16 12:38:14 UTC
Moving several components from Tomcat 6.0.x to 8.5.x I have noticed that #getInfo() is gone. RealmBase has an abstact method #getName() which is used in #toString()(). ValveBase lacks this abstract method. This makes the #toString() implementations inconsistent.

RealmBase:

> @Override
> public String toString() {
>     StringBuilder sb = new StringBuilder("Realm[");
>     sb.append(getName());
>     sb.append(']');
>     return sb.toString();
> }

ValveBase:

> @Override
> public String toString() {
>     StringBuilder sb = new StringBuilder(this.getClass().getName());
>     sb.append('[');
>     if (container == null) {
>         sb.append("Container is null");
>     } else {
>         sb.append(container.getName());
>     }
>     sb.append(']');
>     return sb.toString();
> }

I would rather expect a consistent behavior for both base implementations
Comment 1 Michael Osipov 2016-11-20 19:07:53 UTC
I am willing to provide a patch if we agree on one approach.
Comment 2 Mark Thomas 2016-11-21 13:17:20 UTC
The toString() implementations have been pretty much unchanged since the lifecycle refactoring in 7.0.x. While users shouldn't not be expecting these to be in any particular format, the chances are that there that some users are expecting the current format. Therefore, we can look at this for trunk but I don't think it should be back-ported.

My current thinking for a standardised format is:
SimpleClassName[container.toString()] / SimpleClassName[Container is null]

This could/should apply to any component that is usually associated with a single container (Valve, Realm, Manager, etc.)

If we make more use of the Contained interface, it should be possible to do this as a single utility method e.g.:
o.a.c.util.DebugUtil.containedToString(Contained)

Although, that won't work with Manager, Loader etc. that are Context specific.

Maybe
o.a.c.util.DebugUtil.containedToString(Object, Container)
as well for those objects that don't/can't implement Contained.

Which means we might not need to expand the use of Contained anyway. I need to spend some time thinking about how much sense that does or doesn't make.
Comment 3 Michael Osipov 2016-11-22 09:29:25 UTC
(In reply to Mark Thomas from comment #2)
> The toString() implementations have been pretty much unchanged since the
> lifecycle refactoring in 7.0.x. While users shouldn't not be expecting these
> to be in any particular format, the chances are that there that some users
> are expecting the current format. Therefore, we can look at this for trunk
> but I don't think it should be back-ported.

I do not even expect some specific format, but rather a consistent approach. Don't you think it is worthwile to port back to 8.5? This is going to be supported for a long time compared to 6.0/7.0.

> My current thinking for a standardised format is:
> SimpleClassName[container.toString()] / SimpleClassName[Container is null]

Do you think that the simple class name is sufficient? I was used to #getInfo() previously which has the FQCN, and this method is gone. Consider that people might copy Tomcat's standard component into their source tree, modify code and package but leave class name as-is. Still, this is good compromise.
 
> If we make more use of the Contained interface, it should be possible to do
> this as a single utility method e.g.:
> o.a.c.util.DebugUtil.containedToString(Contained)
>
> Maybe
> o.a.c.util.DebugUtil.containedToString(Object, Container)
> as well for those objects that don't/can't implement Contained.

This make defitively sense!
 
> Which means we might not need to expand the use of Contained anyway. I need
> to spend some time thinking about how much sense that does or doesn't make.

I have noticed that a lot of components which use Container do not implement Contained at all. Is there a legacy reason for that? It seems awkward.

It might be worth considering deprecating RealmBase#getName() since only toString() uses it and it is likely to be superseded.

Moreover, toString() has to be well crafted if it is used in MBeans/JMX or log statements to clearly identify the component itself and its location in the server tree.
Comment 4 Mark Thomas 2016-11-23 10:10:19 UTC
(In reply to Michael Osipov from comment #3)

> Don't you think it is worthwile to port back to 8.5?

No.

> Do you think that the simple class name is sufficient?

Yes.

> I have noticed that a lot of components which use Container do not implement
> Contained at all. Is there a legacy reason for that? It seems awkward.

It was added to solve a specific problem for Valves. It wasn't added to the other components because it wasn't required.

> It might be worth considering deprecating RealmBase#getName() since only
> toString() uses it and it is likely to be superseded.

+1.

> Moreover, toString() has to be well crafted if it is used in MBeans/JMX or
> log statements to clearly identify the component itself and its location in
> the server tree.

JMX name generation should be separate from toString().
Comment 5 Michael Osipov 2016-11-25 09:15:37 UTC
I see that you have picked up my proposal to reuse the Contained interface: http://www.mail-archive.com/dev@tomcat.apache.org/msg113336.html
Comment 6 Mark Thomas 2016-11-25 09:49:16 UTC
Fixed in:
- trunk for 9.0.0.M14 onwards

I also applied the clean-up to Realm.getName()