Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records #3201

Closed
wants to merge 13 commits into from
Closed

8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records #3201

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
bf8b437
8264124: Update MXBean specification and implementation to extend map…
dfuch Mar 25, 2021
1c3292c
Fixed typo. Moved example with record after example with from method …
dfuch Mar 26, 2021
8e5a70c
Integrated review feedback. Added a section for Mapping to records.
dfuch Mar 30, 2021
aaf7449
Integrated review feedback. Updated the section for Mapping to records.
dfuch Mar 31, 2021
4b87469
Merge branch 'master' into mxbeans-8264124
dfuch Mar 31, 2021
2a75f25
Minor tweaks. Improved test.
dfuch Mar 31, 2021
5996513
Merge branch 'master' into mxbeans-8264124
dfuch Mar 31, 2021
7d478de
Moved mapping for records just before Mapping for MXBean interface; I…
dfuch Mar 31, 2021
6e6f9d3
Improved test
dfuch Mar 31, 2021
9227ba5
Integrated review feedback
dfuch Mar 31, 2021
44bbf21
minor style issue
dfuch Mar 31, 2021
a624a76
minor style issue
dfuch Mar 31, 2021
257a6ed
Merge
dfuch Apr 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 132 additions & 29 deletions src/java.management/share/classes/com/sun/jmx/mbeanserver/DefaultMXBeanMappingFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2005, 2015, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2005, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -443,9 +443,11 @@ private MXBeanMapping makeCompositeMapping(Class<?> c,
if (gcInfoHack && propertyName.equals("CompositeType"))
continue;

Method old =
getterMap.put(decapitalize(propertyName),
method);
// Don't decapitalize if this is a record component name.
// We only decapitalize for getXxxx(), isXxxx(), and setXxxx()
String name = c.isRecord() && method.getName().equals(propertyName)
? propertyName : decapitalize(propertyName);
Method old = getterMap.put(name, method);
if (old != null) {
final String msg =
"Class " + c.getName() + " has method name clash: " +
Expand Down Expand Up @@ -862,6 +864,9 @@ private synchronized void makeCompositeBuilder()
{
new CompositeBuilderViaFrom(targetClass, itemNames),
},
{
new RecordCompositeBuilder(targetClass, itemNames),
},
{
new CompositeBuilderViaConstructor(targetClass, itemNames),
},
Expand Down Expand Up @@ -1139,14 +1144,14 @@ Object fromCompositeData(CompositeData cd,
/** Builder for when the target class has a constructor that is
annotated with {@linkplain ConstructorParameters &#64;ConstructorParameters}
or {@code @ConstructorProperties} so we can see the correspondence to getters. */
private static final class CompositeBuilderViaConstructor
private static class CompositeBuilderViaConstructor
extends CompositeBuilder {

CompositeBuilderViaConstructor(Class<?> targetClass, String[] itemNames) {
super(targetClass, itemNames);
}

private String[] getConstPropValues(Constructor<?> ctr) {
String[] getConstPropValues(Constructor<?> ctr) {
// is constructor annotated by javax.management.ConstructorParameters ?
ConstructorParameters ctrProps = ctr.getAnnotation(ConstructorParameters.class);
if (ctrProps != null) {
Expand All @@ -1171,8 +1176,7 @@ && getConstPropValues(constr) != null)
}

if (annotatedConstrList.isEmpty())
return "no constructor has either @ConstructorParameters " +
"or @ConstructorProperties annotation";
return reportNoConstructor();

annotatedConstructors = newList();

Expand All @@ -1196,17 +1200,15 @@ && getConstPropValues(constr) != null)
// so we can test unambiguity.
Set<BitSet> getterIndexSets = newSet();
for (Constructor<?> constr : annotatedConstrList) {
String annotationName =
constr.isAnnotationPresent(ConstructorParameters.class) ?
"@ConstructorParameters" : "@ConstructorProperties";
String matchingMechanism = matchingMechanism(constr);

String[] propertyNames = getConstPropValues(constr);

Type[] paramTypes = constr.getGenericParameterTypes();
if (paramTypes.length != propertyNames.length) {
final String msg =
"Number of constructor params does not match " +
annotationName + " annotation: " + constr;
referenceMechannism(matchingMechanism) +": " + constr;
throw new InvalidObjectException(msg);
}

Expand All @@ -1219,7 +1221,7 @@ && getConstPropValues(constr) != null)
String propertyName = propertyNames[i];
if (!getterMap.containsKey(propertyName)) {
String msg =
annotationName + " includes name " + propertyName +
matchingMechanism + " includes name " + propertyName +
" which does not correspond to a property";
for (String getterName : getterMap.keySet()) {
if (getterName.equalsIgnoreCase(propertyName)) {
Expand All @@ -1234,7 +1236,7 @@ && getConstPropValues(constr) != null)
paramIndexes[getterIndex] = i;
if (present.get(getterIndex)) {
final String msg =
annotationName + " contains property " +
matchingMechanism + " contains property " +
propertyName + " more than once: " + constr;
throw new InvalidObjectException(msg);
}
Expand All @@ -1243,7 +1245,7 @@ && getConstPropValues(constr) != null)
Type propertyType = getter.getGenericReturnType();
if (!propertyType.equals(paramTypes[i])) {
final String msg =
annotationName + " gives property " + propertyName +
matchingMechanism + " gives property " + propertyName +
" of type " + propertyType + " for parameter " +
" of type " + paramTypes[i] + ": " + constr;
throw new InvalidObjectException(msg);
Expand All @@ -1252,10 +1254,7 @@ && getConstPropValues(constr) != null)

if (!getterIndexSets.add(present)) {
final String msg =
"More than one constructor has " +
"@ConstructorParameters or @ConstructorProperties " +
"annotation with this set of names: " +
Arrays.toString(propertyNames);
reportMultipleConstructorsFoundFor(propertyNames);
throw new InvalidObjectException(msg);
}

Expand Down Expand Up @@ -1292,10 +1291,7 @@ else if (seen) {
i = u.nextSetBit(i+1))
names.add(itemNames[i]);
final String msg =
"Constructors with @ConstructorParameters or " +
"@ConstructorProperties annotation " +
"would be ambiguous for these items: " +
names;
reportConstructorsAmbiguousFor(names);
throw new InvalidObjectException(msg);
}
}
Expand All @@ -1305,7 +1301,41 @@ else if (seen) {
return null; // success!
}

final Object fromCompositeData(CompositeData cd,
String reportNoConstructor() {
return "no constructor has either @ConstructorParameters " +
"or @ConstructorProperties annotation";
}

String matchingMechanism(Constructor<?> constr) {
return constr.isAnnotationPresent(ConstructorParameters.class) ?
"@ConstructorParameters" : "@ConstructorProperties";
}

String referenceMechannism(String matchingMechanism) {
return matchingMechanism + " annotation";
}

String reportMultipleConstructorsFoundFor(String... propertyNames) {
return "More than one constructor has " +
"@ConstructorParameters or @ConstructorProperties " +
"annotation with this set of names: " +
Arrays.toString(propertyNames);
}

String reportConstructorsAmbiguousFor(Set<String> names) {
return "Constructors with @ConstructorParameters or " +
"@ConstructorProperties annotation " +
"would be ambiguous for these items: " +
names;
}

String reportNoConstructorFoundFor(Set<String> names) {
return "No constructor has either @ConstructorParameters " +
"or @ConstructorProperties annotation for this set of " +
"items: " + names;
}

Object fromCompositeData(CompositeData cd,
String[] itemNames,
MXBeanMapping[] mappings)
throws InvalidObjectException {
Expand All @@ -1330,10 +1360,7 @@ final Object fromCompositeData(CompositeData cd,
}

if (max == null) {
final String msg =
"No constructor has either @ConstructorParameters " +
"or @ConstructorProperties annotation for this set of " +
"items: " + ct.keySet();
final String msg = reportNoConstructorFoundFor(ct.keySet());
throw new InvalidObjectException(msg);
}

Expand Down Expand Up @@ -1379,6 +1406,73 @@ private static class Constr {
private List<Constr> annotatedConstructors;
}

/** Builder for when the target class is a record */
private static final class RecordCompositeBuilder
extends CompositeBuilderViaConstructor {

RecordCompositeBuilder(Class<?> targetClass, String[] itemNames) {
super(targetClass, itemNames);
}

String[] getConstPropValues(Constructor<?> ctor) {
var components = getTargetClass().getRecordComponents();
var ptypes = ctor.getGenericParameterTypes();
if (components.length != ptypes.length) {
return super.getConstPropValues(ctor);
}
var len = components.length;
String[] res = new String[len];
for (int i=0; i < len ; i++) {
if (!ptypes[i].equals(components[i].getGenericType())) {
return super.getConstPropValues(ctor);
}
res[i] = components[i].getName();
}
return res;
}

String applicable(Method[] getters) throws InvalidObjectException {
Class<?> targetClass = getTargetClass();
if (!targetClass.isRecord())
return "class is not a record";

return super.applicable(getters);
}

@Override
Object fromCompositeData(CompositeData cd, String[] itemNames, MXBeanMapping[] mappings)
throws InvalidObjectException {
return super.fromCompositeData(cd, itemNames, mappings);
}

String reportNoConstructor() {
return "canonical constructor for record not found";
}

String matchingMechanism(Constructor<?> constr) {
return "canonical constructor";
}

String referenceMechannism(String matchingMechanism) {
return matchingMechanism;
}

String reportMultipleConstructorsFoundFor(String... propertyNames) {
return "More than one constructor has this set of names: " +
Arrays.toString(propertyNames);
}

String reportConstructorsAmbiguousFor(Set<String> names) {
return "Constructors would be ambiguous for these items: " +
names;
}

String reportNoConstructorFoundFor(Set<String> names) {
return "No constructor has this set of " +
"items: " + names;
}
}

/** Builder for when the target class is an interface and contains
no methods other than getters. Then we can make an instance
using a dynamic proxy that forwards the getters to the source
Expand Down Expand Up @@ -1504,7 +1598,16 @@ static String capitalize(String name) {
public static String propertyName(Method m) {
String rest = null;
String name = m.getName();
if (name.startsWith("get"))
var c = m.getDeclaringClass();
if (c.isRecord()) {
for (var rc : c.getRecordComponents()) {
if (name.equals(rc.getName())
&& m.getReturnType() == rc.getType()) {
rest = name;
break;
}
}
} else if (name.startsWith("get"))
rest = name.substring(3);
else if (name.startsWith("is") && m.getReturnType() == boolean.class)
rest = name.substring(2);
Expand Down
34 changes: 31 additions & 3 deletions src/java.management/share/classes/javax/management/MXBean.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2005, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2005, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -753,7 +753,9 @@ another proxy (like {@code ModuleMXBean.getProduct()} which
{@code CompositeType} is determined by the <a href="#type-names">
type name rules</a> below.</p>

<p>The class is examined for getters using the conventions
<p>If the class is a {@link Record}, its getters are the
accessors for the record components. Otherwise, the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be good to add a link like {@linkplain RecordComponent record components}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You add record in "Mappings for other types". I think it deserves a separate section "Mappings for record classes" (maybe after primitive types). It's useful to add a row for record in the summary table above "Mappings for primitive types"

Copy link
Member Author

@dfuch dfuch Mar 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link added. With respect to adding a section for records, it's a bit difficult to separate that from the "Mapping for other types" without a lot of duplication. I can add a row in the summary table, and then add a new section "Mapping for records" just before "Mapping for other types", that just gives a brief overview and refers to the mapping for other types below.

Something like this - what do you think?

    <h3 id="records">Mappings for Records</h3>

    <p>A {@linkplain Record record} <em>R</em> whose {@linkplain
      Class#getRecordComponents() components} are
      all convertible to open types, is itself convertible to a
      {@link CompositeType} as follows.
      The type name of this {@code CompositeType}
      is determined by the same <a href="#type-names">type name rules</a>
      defined by the <a href="#composite-map">Mapping for other types</a>
      below. Its getters are the accessors for the {@linkplain
      RecordComponent record components}, and the record is reconstructed
      using its canonical constructor, without needing any annotation.</p>

    <p>A record may also expose additional non-canonical constructors, which
      can be used to reconstruct the record if the composite data does
      not exactly contain all the components expected by the
      canonical constructor. However, in order to be taken into account
      by the MXBean framework, such non-canonical constructors
      need to be annotated with either the {@link ConstructorParameters
      &#64;javax.management.ConstructorParameters} or
     {@code @java.beans.ConstructorProperties} annotation.</p>

    <p>The complete rules for the mapping are detailed as part
      of the <a href="#composite-map">Mapping for other types</a>
      below.</p>

    <h3 id="composite-map">Mappings for other types</h3>

    <p>Given a record, or a Java class or interface <em>J</em> that does not match the other
      rules in the table above, the MXBean framework will attempt to map
      it to a {@link CompositeType} as follows.  [....]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Mandy, I have updated the PR with a revised version of the text above. Is that more in line what you had in mind?

best regards,
-- daniel

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks better. I like a separate mapping section for records since the mapping rules are straight-forward. The complexity comes if we want to support a record to implement CompositeDataView that allows a type to support more flexible conversion to CompositeData which is why you add records in the "Mappings for other types". I am wondering if we really want to support records to implement CompositeDataView but we may want to avoid such a special case.

I suggest add subsections in "Mappings for other types": There are two mapping rules: (1) opentype(J) maps J to CompositeType (2) opendata(J) maps J to CompositeData. opentype(J) for record class does not need to refer to other types. The common cases for opendata(J) for record class is using the canonical constructors. The other less common cases like using non-canonical constructors and CompositeDataView can state that it follows the same rules as specified for other types. "Mappings for other types" does not need any change for records.

"Reconstructing an instance of Java type J from a CompositeData" section should be renamed to "Reconstructing an instance of Java type or record class J from a CompositeData"

Here is a minor tweaking on "Mappings for Records"

A record is convertible to a CompositeType if and only if all its components are 
convertible to open types. Otherwise, it is not convertible.

A record class is converted to a CompositeType with one item for every record component as follows.
- The type name of this CompositeType is determined by the type name rules detailed below. 
- Each record component of type T, the item in the CompositeType has the same name 
  as the record component and of type opentype(T).

The mapping from an instance of a record class to a CompositeData corresponding to 
the CompositeType is the same as specified as for other types (add a link).

A record is reconstructed from a CompositeData using its canonical constructor. 
The canonical constructor doesn't require the presence of @javax.management.ConstructorParameters 
or @java.beans.ConstructorProperties annotations. If these annotations are present on
the canonical constructor, they will be ignored.

If the CompositeData from which the record is reconstructed doesn't contain all the record components, 
the MXBean framework will attempt to reconstruct the record in the same way as specified for 
other types (add a link).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the PR with those changes, plus a few minor adjustments.

class is examined for getters using the conventions
<a href="#naming-conv">above</a>. (Getters must be public
instance methods.) If there are no getters, or if
any getter has a type that is not convertible, then <em>J</em> is
Expand Down Expand Up @@ -784,7 +786,13 @@ <code>String getOwner()</code>
then the item in the {@code CompositeType} is called {@code name}
and has type {@code SimpleType.BOOLEAN}.

<p>Notice that the first character (or code point) is converted to
<p>If the class is a {@link Record} and the getter is a component
accessor for a record component {@code name} of type <em>T</em>,
then the item in the {@code CompositeType} has the same name
as the record component, and has type <em>opentype(T)</em>.</p>

<p>Notice that unless the class is a {@code Record}, the
first character (or code point) is converted to
lower case. This follows the Java Beans convention, which for
historical reasons is different from the Standard MBean
convention. In a Standard MBean or MXBean interface, a method
Expand Down Expand Up @@ -846,6 +854,17 @@ implements the interface {@link CompositeDataView}, then that
then that method is called to reconstruct an instance of
<em>J</em>.</p></li>

<li><p>Otherwise, if <em>J</em> is a {@link Record} class,
and the record canonical constructor is applicable,
an instance of <em>J</em> is reconstructed by calling
the record canonical constructor.
The canonical constructor, if applicable, is called
with the appropriate reconstructed items from the
{@code CompositeData}. The canonical constructor
is <em>applicable</em> if all the properties named
by the record components are present in the
{@code CompositeData}.</p></li>

<li><p>Otherwise, if <em>J</em> has at least one public
constructor with either {@link javax.management.ConstructorParameters
&#64;javax.management.ConstructorParameters} or
Expand Down Expand Up @@ -962,6 +981,15 @@ <b>public static NamedNumber from(CompositeData cd)</b> {
</blockquote>
</li>

<li>Record:

<blockquote>
<pre>
public record NamedNumber(int number, String name) {}
</pre>
</blockquote>
</li>

<li>Public constructor with <code>&#64;ConstructorParameters</code> annotation:

<blockquote>
Expand Down
Loading