osgi / bugzilla-archive

Archive of OSGi Alliance Specification Bugzilla bugs. The Specification Bugzilla system was decommissioned with the move to GitHub. The issues in this repository are imported from the Specification Bugzilla system for archival purposes.
0 stars 1 forks source link

[converter] Special support for single-element annotations #2835

Closed bjhargrave closed 7 years ago

bjhargrave commented 7 years ago

Original bug ID: BZ#2967 From: @rotty3000 Reported version: R7

bjhargrave commented 7 years ago

Comment author: @rotty3000

Section 707.4.4.4 Annotation of the Converter spec discusses Annotations.

It explains that annotations are treated exactly like interfaces however there is one significant difference.

Simple annotations contain only the value() property and this case is treated slightly differently.

Should consideration be made for this case? Multiple such annotations on a single annotation target could result in many "value=###" key/value pairs and these can collide and possible lose meaningful information which is actually defined by the annotation type name.

bjhargrave commented 7 years ago

Comment author: @bjhargrave

I added a new KeyName annotation to the converter. https://osgi.org/gitweb/build.git/blob/refs/heads/core:/org.osgi.util.converter/src/org/osgi/util/converter/annotations/KeyName.java.

The @ KeyName annotation can be applies to members of types supported by the Converter. Such as interface methods, annotation elements, and DTO fields.

At runtime, the Converter must look for the runtime retention KeyName annotation on the member. If present, the Converter must use the KeyName annotation's value for the property key name. If not present, the Converter will apply the member name "mangling" rules to determine the property key name.

Since the converter needs to access the annotation at runtime, the annotation is a runtime retention annotation. This means that at runtime, bundle's having types using the KeyName annotation must import the KeyName annotation's package.

I also updated the DS property types (e.g. ServiceDescription) to use the @ KeyName annotation on their value() elements.

bjhargrave commented 7 years ago

Comment author: @bosschaert

Reopening and assigning to myself for the spec text in relation to this.

bjhargrave commented 7 years ago

Comment author: @bjhargrave

Thinking about this some more, we could instead do a more limited form of support for "simple" annotations. We define a simple annotation as an annotation having one element whose name is "value". For such annotations we could mangle the value member name to a property key name based up on the annotation type name. Since Java type naming convention is CamelCase, the mangle rules would be to insert a full stop before each Uppercase letter (except at the start the name) and convert all Uppercase letters to lowercase.

For example:

public @ interface ServiceRanking { int value(); }

The key name for the value element is "service.ranking".

While this is much less expressive than the current @ KeyName proposal, it has several advantages. One, it does not require the annotation author to apply the @ KeyName annotation which means it will be work on all existing "simple" annotations. Two, the converter does not need to do any runtime annotation processing which means that bundles do not need to import the org.osgi.util.converter.annotations package. I am very concerned about bundles having to import this package. There will be a subtle source of errors regarding this package. Failing to import that package will never produce an obvious ClassDefNotFoundError. Instead, the converter will look for the @ KeyName annotation on the member and not find it since the JVM will just not reify the annotation on the member as the annotation class is not available. So the conversion will be wrong as the member name to key name mapping will be wrong.

I think I am now leaning to backing out the KeyName annotation and just using the convention for "simple" annotations.

bjhargrave commented 7 years ago

Comment author: @rotty3000

(In reply to BJ Hargrave from comment BZ#3)

Thinking about this some more, we could instead do a more limited form of support for "simple" annotations. We define a simple annotation as an annotation having one element whose name is "value". For such annotations we could mangle the value member name to a property key name based up on the annotation type name. Since Java type naming convention is CamelCase, the mangle rules would be to insert a full stop before each Uppercase letter (except at the start the name) and convert all Uppercase letters to lowercase.

For example:

public @ interface ServiceRanking { int value(); }

The key name for the value element is "service.ranking".

While this is much less expressive than the current @ KeyName proposal, it has several advantages. One, it does not require the annotation author to apply the @ KeyName annotation which means it will be work on all existing "simple" annotations. Two, the converter does not need to do any runtime annotation processing which means that bundles do not need to import the org.osgi.util.converter.annotations package. I am very concerned about bundles having to import this package. There will be a subtle source of errors regarding this package. Failing to import that package will never produce an obvious ClassDefNotFoundError. Instead, the converter will look for the @ KeyName annotation on the member and not find it since the JVM will just not reify the annotation on the member as the annotation class is not available. So the conversion will be wrong as the member name to key name mapping will be wrong.

I think I am now leaning to backing out the KeyName annotation and just using the convention for "simple" annotations.

I agree with this latest conclusion!

I think particularly in the face of adapting existing Annotation based technologies to OSGi, of which we cannot easily change existing APIs, this last proposal would simplify things greatly. I think it's a straight forward convention that people can fairly easily adjust to.

bjhargrave commented 7 years ago

Comment author: @bjhargrave

OK. The JLS defines the term "single-element annotation". See https://docs.oracle.com/javase/specs/jls/se7/html/jls-9.html#jls-9.7.3. Basically, a single-element annotation is an annotation with a value() element. A single-element annotation can have additional elements, but all additional elements must have default values.

Thus a single-element annotation can always be used like:

@ Foo(100)

rather than having to specify the element name:

@ Foo(value=100)

So we change the Converter spec to have special treatment for single-element annotations. For single-element annotations, the key name for the value() element is computed differently than for other elements. For single-element annotations, the key name for the value() element is based upon the annotation type name. Since Java type naming convention is CamelCase, the mangle rule is to insert a full stop ('.') before each Uppercase letter (except at the start the name) and convert all Uppercase letters to lowercase.

For example:

public @ interface ServiceRanking { int value(); }

The key name for the value() element is "service.ranking".

I also updated the DS property types (e.g. ServiceDescription) to use value() elements.

bjhargrave commented 7 years ago

Comment author: @bjhargrave

Method to test if a class is a single-element annotation type:

public static boolean isSingleElementAnnotation(Class< ? > clazz) { if (!clazz.isAnnotation()) { return false; } boolean result = false; Method[] methods = clazz.getDeclaredMethods(); for (Method m : methods) { if (m.getName().equals("value")) { result = true; } else { try { if (m.getDefaultValue() == null) { return false; } } catch (TypeNotPresentException e) { // has a default, but we cannot load it } } } return result; }

bjhargrave commented 7 years ago

Comment author: @bjhargrave

Some tweaks to the single-element annotation type name to key name mapping rule:

public static String mapSingleElementAnnotationName(Class< ? > clazz) { StringBuilder sb = new StringBuilder(clazz.getSimpleName()); boolean lastLowerCase = false; for (int i = 0; i < sb.length(); i++) { char c = sb.charAt(i); if (Character.isUpperCase(c)) { sb.setCharAt(i, Character.toLowerCase(c)); if (lastLowerCase) { sb.insert(i, '.'); } lastLowerCase = false; } else { lastLowerCase = Character.isLowerCase(c); } } return sb.toString(); }

The rule uses the simple name of the type to avoid the package name and outer class name if the type is an inner class.

A full stop is only inserted at the transition boundary of lowercase letter followed by an Uppercase letter. This lets type names like "OSGiThing" be mapped to "osgi.thing" instead of "o.s.g.i.thing".

bjhargrave commented 7 years ago

Comment author: @bjhargrave

(In reply to BJ Hargrave from comment BZ#7)

          sb.insert(i, '.');

sb.insert(i++, '.');

bjhargrave commented 7 years ago

Comment author: @bjhargrave

I added support for single-element annotations to the DS 1.4 spec.

bjhargrave commented 7 years ago

Comment author: @bjhargrave

I also need to add support to the Metatype spec.

bjhargrave commented 7 years ago

Comment author: @bjhargrave

(In reply to BJ Hargrave from comment BZ#10)

I also need to add support to the Metatype spec.

Support added to the Metatype spec which is now at version 1.4.

bjhargrave commented 7 years ago

Comment author: @bosschaert

The spec and RI support this now.