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

Fix incorrect code generated by the -Xpropertyccessors option #1286

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix incorrect code generated by the -Xpropertyccessors option #1286

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d602c8b
Fix incorrect code generated by the -Xgenerateaccessors option
pwilkin Jan 29, 2019
1ad1d9b
Sign off commit for ECA
pwilkin Feb 1, 2019
52ae8b4
Signoff
pwilkin Feb 1, 2019
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
7 changes: 7 additions & 0 deletions jaxb-ri/codemodel/codemodel/src/main/java/com/sun/codemodel/JMethod.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,13 @@ public JAnnotationUse annotate(JClass clazz){
annotations.add(a);
return a;
}

public JAnnotationUse annotate(JAnnotationUse existingAnnotation) {
if (annotations == null)
annotations = new ArrayList<JAnnotationUse>();
annotations.add(existingAnnotation);
return existingAnnotation;
}

/**
* Adds an annotation to this variable.
Expand Down
90 changes: 80 additions & 10 deletions jaxb-ri/xjc/src/main/java/com/sun/tools/xjc/addon/accessors/PluginImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,27 +10,44 @@

package com.sun.tools.xjc.addon.accessors;

import com.sun.codemodel.JAnnotationUse;
import com.sun.codemodel.JClass;
import java.io.IOException;
import com.sun.tools.xjc.BadCommandLineException;
import com.sun.tools.xjc.Options;
import com.sun.tools.xjc.Plugin;
import com.sun.tools.xjc.outline.ClassOutline;
import com.sun.tools.xjc.outline.Outline;
import java.io.StringWriter;
import java.lang.reflect.Field;
import java.util.Collection;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Set;
import java.util.logging.Level;
import java.util.logging.Logger;

import javax.xml.bind.annotation.XmlAccessType;
import javax.xml.bind.annotation.XmlAccessorType;
import javax.xml.bind.annotation.XmlElement;

import org.xml.sax.ErrorHandler;

import com.sun.codemodel.JAnnotationUse;
import com.sun.codemodel.JAnnotationValue;
import com.sun.codemodel.JClass;
import com.sun.codemodel.JFieldVar;
import com.sun.codemodel.JFormatter;
import com.sun.codemodel.JMethod;
import com.sun.codemodel.JType;
import com.sun.tools.xjc.BadCommandLineException;
import com.sun.tools.xjc.Options;
import com.sun.tools.xjc.Plugin;
import com.sun.tools.xjc.outline.ClassOutline;
import com.sun.tools.xjc.outline.Outline;
import com.sun.xml.bind.api.impl.NameConverter;

/**
* Generates synchronized methods.
* Generates elements via property accessors.
*
* @author
* Martin Grebac ([email protected])
* @author
* Piotr Wilkin ([email protected])
*
*/
public class PluginImpl extends Plugin {

Expand All @@ -47,9 +64,10 @@ public int parseArgument(Options opt, String[] args, int i) throws BadCommandLin
return 0; // no option recognized
}

public boolean run( Outline model, Options opt, ErrorHandler errorHandler ) {
@SuppressWarnings("unchecked")
public boolean run( Outline model, Options opt, ErrorHandler errorHandler ) {

for( ClassOutline co : model.getClasses() ) {
for( ClassOutline co : (Collection<ClassOutline>) model.getClasses() ) {
Iterator<JAnnotationUse> ann = co.ref.annotations().iterator();
while (ann.hasNext()) {
try {
Expand All @@ -71,6 +89,58 @@ public boolean run( Outline model, Options opt, ErrorHandler errorHandler ) {
Logger.getLogger(PluginImpl.class.getName()).log(Level.SEVERE, null, ex);
}
}
Iterator<JFieldVar> flds = co.implClass.fields().values().iterator();
while (flds.hasNext()) {
try {
JFieldVar fld = flds.next();
String fldName = fld.name();
Set<JAnnotationUse> annotsCopy = new HashSet<JAnnotationUse>(fld.annotations());
String getterName = "get" + fldName.substring(0, 1).toUpperCase() + fldName.substring(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can there be "is" instead of "get" for booleans?

Copy link
Author

Choose a reason for hiding this comment

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

There probably could, but note that this is not what the plugin does. The plugin doesn't generate the getters, it merely finds the appropriate ones to insert the annotations. If you want to change the generated names, you should probably direct that to the maintainer of the code that actually generates the getters.

Copy link
Contributor

Choose a reason for hiding this comment

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

You won't find appropriate getters if you are searching by the wrong name. Consider following property:

private boolean positive;
public boolean isPositive();
public setPositive(boolean positive);

In that case it will skip moving annotations from field to method.

String escapedName = null;
for (JAnnotationUse annot : annotsCopy) {
Field clazzField = annot.getClass().getDeclaredField("clazz");
clazzField.setAccessible(true);
JClass cl = (JClass) clazzField.get(annot);
if (cl.equals(model.getCodeModel()._ref(XmlElement.class))) {
if (annot.getAnnotationMembers().containsKey("name")) {
JAnnotationValue val = (JAnnotationValue) annot.getAnnotationMembers().get("name");
StringWriter sw = new StringWriter();
JFormatter ft = new JFormatter(sw);
val.generate(ft);
String quotedName = sw.getBuffer().toString();
String name = quotedName.substring(quotedName.indexOf('"') + 1, quotedName.lastIndexOf('"'));
escapedName = NameConverter.standard.toClassName(name);
getterName = "get" + escapedName;
}
}
}

JMethod getterMethod = co.implClass.getMethod(getterName, new JType[] {});
if (getterMethod != null) {
for (JAnnotationUse annot : annotsCopy) {
fld.removeAnnotation(annot);
getterMethod.annotate(annot);
}
String properGetterName = "get" + fldName.substring(0, 1).toUpperCase() + fldName.substring(1);
if (!properGetterName.equals(getterName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for normalizing names? When does it happen that field / getter does not match by javabean convention?

Copy link
Author

Choose a reason for hiding this comment

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

Again, I do not know. I guess this has something to do with the convention that Java class property names are lowercase, so uppercase-named elements are reduced to lowercase properties and then the getter gets the appropriate name by convention.

Copy link
Contributor

Choose a reason for hiding this comment

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

My point is not about javabean convention, it is why there is that particular code. First property name is read from field name. Second property name is reread from XmlElement("name") value (if present). Than method is looked up by this name, and if found, name is recalculated according to field name and rewritten. Lets consider following example:

@XmlElement(name = "abc")
private Long id;
public getId();
public void setId(Long id);
  • getterName is first set to "getId" by the field name + getPrefix
  • getterName is reset to name from XmlElement annotation and is now getabc (uppercasing 'a' looks to be omitted here).
  • next we are looking for getabc which is not present in the class and the code actually exits without doing anyting for id field.

Following will happen after, only when XmlElement(name) is not set or is set to the field name:

  • Found getter by getId name
  • Move annotations from field to getter
  • Recalculate properGetterName by the field name to getId and overwrite method name to properGetterName (same for setter)

Either, I don't understand what the code is doing, which is quite possible due to missing tests to prove it, or the code is working only partially and needs revisiting.

// normalize the getter/setter names
getterMethod.name(properGetterName);
String setterName = "set" + escapedName;
JMethod setterMethod = co.implClass.getMethod(setterName, new JType[] { getterMethod.type() });
String propperSetterName = "set" + fldName.substring(0, 1).toUpperCase() + fldName.substring(1);
setterMethod.name(propperSetterName);
}
}
} catch (IllegalArgumentException ex) {
Logger.getLogger(PluginImpl.class.getName()).log(Level.SEVERE, null, ex);
} catch (SecurityException ex) {
Logger.getLogger(PluginImpl.class.getName()).log(Level.SEVERE, null, ex);
} catch (NoSuchFieldException e) {
Logger.getLogger(PluginImpl.class.getName()).log(Level.SEVERE, null, e);
} catch (IllegalAccessException e) {
Logger.getLogger(PluginImpl.class.getName()).log(Level.SEVERE, null, e);
}
}
}
return true;
}
Expand Down