Skip to content

Fix generics and some formatting #59

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

Open
dwalluck wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix generics and some formatting #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
64 changes: 23 additions & 41 deletions rwx-processor/src/main/resources/groovy/Parser.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,65 +18,47 @@ import java.util.Map;
public class ${simpleClassName}_Parser implements Parser<${simpleClassName}>
{
@Override
@SuppressWarnings( "unchecked" )
public ${simpleClassName} parse( Object object )
{
${simpleClassName} ret = new ${simpleClassName}();
Object val;

<% if (structPart == true) { %>
Map<String, Object> map = (Map) object;
<% params.each { %>
<% if (structPart == true) { %>
Map<?, ?> map = (Map<?, ?>) object;
<% params.each { %>
val = map.get( "${it.key}" );
if ( val != null )
{
{
val = nullifyNil( val );
<% if (it.converter != null) { %>
ret.${it.methodName}( new ${it.converter}().parse( val ) );
<% } else if (it.actionClass == null) { %>
<% if (it.isPrimitive) { %>if ( val != null ) <% } %>ret.${it.methodName}( (${it.type}) <% if (it.isUpgradeCast) { %>upgradeCast( ${it.type}.class, val )<% } else { %>val<% } %> );
<% } else { %>
<% if (it.contains) { %>
List<${it.elementClass}> ${it.localListVariableName} = new ArrayList<>();
for ( Object obj : ( List<Object> ) val )
<% if (it.converter != null) { %> ret.${it.methodName}( new ${it.converter}().parse( val ) );
<% } else if (it.actionClass == null) { %> <% if (it.isPrimitive) { %>if ( val != null ) <% } %>ret.${it.methodName}( <% if (it.type != 'java.lang.Object') { %>(${it.type}) <% } %><% if (it.isUpgradeCast) { %>upgradeCast( ${it.type}.class, val )<% } else { %>val<% } %> );
<% } else if (it.contains) { %> List<${it.elementClass}> ${it.localListVariableName} = new ArrayList<>();
for ( Object obj : (List<?>) val )
{
${it.localListVariableName}.add( new ${it.actionClass}().parse( obj ) );
}
ret.${it.methodName}( ${it.localListVariableName} );
<% } else { %>
ret.${it.methodName}( new ${it.actionClass}().parse( val ) );
<% } %>
<% } %>
}
<% } %>
<% } else { %>
<% if (arrayPart == true) { %>
List<Object> list = (List)object;
<% } else { %>
List<Object> list = ((RpcObject) object).getParams();
<% } %>
<% params.eachWithIndex { it, idx -> %>
<% } else { %> ret.${it.methodName}( new ${it.actionClass}().parse( val ) );
<% } %> }
<% } %><% } else { %><% if (arrayPart == true) { %>
List<?> list = (List<?>) object;
<% } else { %>
List<?> list = ((RpcObject) object).getParams();
<% } %><% params.eachWithIndex { it, idx -> %>
val = list.get( ${idx} );
if ( val != null && !isNil( val ) )
{
<% if (it.converter != null) { %>
ret.${it.methodName}( new ${it.converter}().parse( val ) );
<% } else if (it.actionClass == null) { %>
ret.${it.methodName}( (${it.type}) <% if (it.isUpgradeCast) { %>upgradeCast( ${it.type}.class, val )<% } else { %>val<% } %> );
<% } else { %>
<% if (it.contains) { %>
List<${it.elementClass}> ${it.localListVariableName} = new ArrayList<>();
for ( Object obj : ( List<Object> ) val )
<% if (it.converter != null) { %> ret.${it.methodName}( new ${it.converter}().parse( val ) );
<% } else if (it.actionClass == null) { %> ret.${it.methodName}( <% if (it.type != 'java.lang.Object') { %>(${it.type}) <% } %><% if (it.isUpgradeCast) { %>upgradeCast( ${it.type}.class, val )<% } else { %>val<% } %> );
<% } else if (it.contains) { %> List<${it.elementClass}> ${it.localListVariableName} = new ArrayList<>();
for ( Object obj : (List<?>) val )
{
${it.localListVariableName}.add( new ${it.actionClass}().parse( obj ) );
}
ret.${it.methodName}( ${it.localListVariableName} );
<% } else { %>
ret.${it.methodName}( new ${it.actionClass}().parse( val ) );
<% } %>
<% } %>
}
<% } %>
<% } %>
<% } else { %> ret.${it.methodName}( new ${it.actionClass}().parse( val ) );
<% } %> }
<% } %><% } %>
return ret;
}
}
15 changes: 5 additions & 10 deletions rwx-processor/src/main/resources/groovy/Registry.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,17 @@ package ${packageName};

import org.commonjava.rwx.core.Registry;

<% imports.each { %>
import ${it};<% } %>

<% imports.each { %>import ${it};
<% } %>
/**
* Created by RWX AnnoProcessor.
*/
public class ${registrySimpleClassName}
public final class ${registrySimpleClassName}
extends Registry
{

public ${registrySimpleClassName}()
{
<% classes.each { %>
setRenderer( ${it}.class, new ${it}_Renderer() );
<% classes.each { %> setRenderer( ${it}.class, new ${it}_Renderer() );
setParser( ${it}.class, new ${it}_Parser() );
<% } %>
}

<% } %> }
}
74 changes: 23 additions & 51 deletions rwx-processor/src/main/resources/groovy/Renderer.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,76 +19,48 @@ public class ${simpleClassName}_Renderer implements Renderer<${simpleClassName}>
@Override
public Object render( ${simpleClassName} object )
{
<% if (request == true) { %>
MethodCall methodCall = new MethodCall();
<% if (request == true) { %> MethodCall methodCall = new MethodCall();
methodCall.setMethodName( "${methodName}" );
<% } else if (response == true) { %>
MethodResponse methodResponse = new MethodResponse();
<% } %>

<% if (structPart == true) { %>
Map<String, Object> map = new HashMap<>();
<% params.each { %>
<% if (it.converter != null) { %>
map.put( "${it.key}", new ${it.converter}().render( object.${it.methodName}() ) );
<% } else if (it.actionClass == null) { %>
map.put( "${it.key}", object.${it.methodName}() );
<% } else { %>
if ( object.${it.methodName}() != null )
<% } else if (response == true) { %> MethodResponse methodResponse = new MethodResponse();

<% } %><% if (structPart == true) { %> Map<String, Object> map = new HashMap<>();
<% params.each { %><% if (it.converter != null) { %> map.put( "${it.key}", new ${it.converter}().render( object.${it.methodName}() ) );
<% } else if (it.actionClass == null) { %> map.put( "${it.key}", object.${it.methodName}() );
<% } else { %> if ( object.${it.methodName}() != null )
{
<% if (it.contains) { %>
List<Object> ${it.localListVariableName} = new ArrayList<>( );
<% if (it.contains) { %> List<Object> ${it.localListVariableName} = new ArrayList<>();
for ( ${it.elementClass} obj : object.${it.methodName}() )
{
${it.localListVariableName}.add( new ${it.actionClass}().render( obj ) );
}
map.put( "${it.key}", ${it.localListVariableName} );
<% } else { %>
map.put( "${it.key}", new ${it.actionClass}().render( object.${it.methodName}() ) );
<% } %>
}
<% } %>
<% } %>
<% } else { %>
List<Object> list = new ArrayList<>();
<% params.each { %>
<% if (it.converter != null) { %>
list.add( new ${it.converter}().render( object.${it.methodName}() ) );
<% } else if (it.actionClass == null) { %>
list.add( object.${it.methodName}() );
<% } else { %>
if ( object.${it.methodName}() != null )
<% } else { %> map.put( "${it.key}", new ${it.actionClass}().render( object.${it.methodName}() ) );
<% } %> }
<% } %><% } %><% } else { %> List<Object> list = new ArrayList<>();
<% params.each { %><% if (it.converter != null) { %> list.add( new ${it.converter}().render( object.${it.methodName}() ) );
<% } else if (it.actionClass == null) { %> list.add( object.${it.methodName}() );
<% } else { %> if ( object.${it.methodName}() != null )
{
<% if (it.contains) { %>
List<Object> ${it.localListVariableName} = new ArrayList<>( );
<% if (it.contains) { %> List<Object> ${it.localListVariableName} = new ArrayList<>();
for ( ${it.elementClass} obj : object.${it.methodName}() )
{
${it.localListVariableName}.add( new ${it.actionClass}().render( obj ) );
}
list.add( ${it.localListVariableName} );
<% } else { %>
list.add( new ${it.actionClass}().render( object.${it.methodName}() ) );
<% } %>
}
<% } else { %> list.add( new ${it.actionClass}().render( object.${it.methodName}() ) );
<% } %> }
else
{
list.add( null );
}
<% } %>
<% } %>
<% } %>

<% if (request == true) { %>
methodCall.setParams( list );
<% } %><% } %><% } %>
<% if (request == true) { %> methodCall.setParams( list );
return methodCall;
<% } else if (response == true) { %>
methodResponse.setParams( list );
<% } else if (response == true) { %> methodResponse.setParams( list );
return methodResponse;
<% } else if (arrayPart == true) { %>
return list;
<% } else { %>
map.values().removeAll( java.util.Collections.singleton( null ) );
<% } else if (arrayPart == true) { %> return list;
<% } else { %> map.values().removeAll( java.util.Collections.singleton( null ) );
return map;
<% } %>
}
<% } %> }
}
2 changes: 1 addition & 1 deletion rwx-test/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,5 +58,5 @@
</plugin>
</plugins>
</build>

</project>
4 changes: 2 additions & 2 deletions rwx-test/src/main/java/org/commonjava/rwx/test/jira/JiraServerInfoConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/**
/*
* Copyright (C) 2010 Red Hat, Inc. (http://github.com/Commonjava/commonjava)
*
* Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -29,7 +29,7 @@ public class JiraServerInfoConverter implements Converter<AbstractJiraServerInfo
@Override
public AbstractJiraServerInfo parse( Object object )
{
Map<String, Object> map = (Map<String, Object>) object;
Map<?, ?> map = (Map<?, ?>) object;

@rnc rnc Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I still think it would be better to use @SuppressWarnings( "unchecked" ) here rather than the wildcard parameterised type ; this would then match your changes in e.g. Registry.java and ParseUtils.

@dwalluck dwalluck Aug 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I have a simple way to explain it. The problem is where the cast is (first line vs. second line):

Wrong way:

Map<String, Integer> map = (Map<String, Integer>) obj; // wrong: unchecked cast
Integer i = map.get( "x" ); // compiler can't verify this, can throw ClassCastException

Right way:

Map<?, ?> map = (Map<?, ?>) obj;
Integer i = (Integer) map.get( "x" ) // checked cast here

When I use @SuppressWarnings("unchecked") in Registry and ParseUtils, that was where it was safe.

In other words, cast the get not the Map.

@dwalluck dwalluck Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See also #48.

@rnc rnc Aug 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Full disclosure - Bob helped here!
You're right in that the general principal is valid ( i.e. Casting individual values is safer than casting the container ). However every value is already cast individually below. So changing the map declaration from Map<String, Object> to Map adds nothing to safety.
What the change does introduce is an asymmetry: render() in the same class still declares Map<String, Object> on line 49, since it constructs the map and knows its types. Having parse() use Map for the same logical structure makes the two halves of the same converter inconsistent.

A narrowly scoped @SuppressWarnings("unchecked") on the single cast line — as you did in ParseUtils — would acknowledge the one genuinely unchecked operation, preserve the Map<String, Object> type for the rest of the method, and keep parse() and render() symmetric. That is why I still think it is the better approach for this particular class.

@dwalluck dwalluck Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

True, there is an asymmetry there, but it's intended: render() calls put(String, Object), so it needs to know the type, while parse() uses only plain Object. The symmetry before came from parse() asserting a type that the compiler can't verify, which is exactly the thing to eliminate.

The cases in ParseUtils and Registry are cases where wildcards can't be used. It's not an open choice. The warnings could not be eliminated there, which is why I kept @SuppressWarnings there.

After this PR, there will be no instances of (Map<String, Object>) left in the codebase. Right now, this line, JiraServerInfoConverter.java:32, seems to be the only one in dispute. Combined with #48, this will eliminate all cases of (Map<String, Object>) as well as raw Map and List that were in the codebase. One aside is that the warnings in the generated code were also being passed onto all consumers like kojiji.

String version = (String) map.get( "version" );
String baseUrl = (String) map.get( "baseUrl" );
String buildDate = (String) map.get( "buildDate" );
Expand Down
6 changes: 3 additions & 3 deletions rwx-test/src/test/java/org/commonjava/rwx/test/koji/KojiMultiCallTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,15 +167,15 @@ private void assertMultiCallResponse( MultiCallResponse response )
assertEquals( "org.dashbuilder-dashbuilder-parent-metadata", kojiBuildInfo.getPackageName() );

// if we do not know the type, access Map directly
Map<String, Object> data1Map = (Map<String, Object>) data1;
Map<?, ?> data1Map = (Map<?, ?>) data1;

@rnc rnc Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you explain why you've gone from a typed string/object to wildcards?

@dwalluck dwalluck Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The goal was to correct the code and fix about 50+ compiler warnings (that show up in IDEA, but only 1 "uses unchecked or unsafe operations" shows in console without -Xlint:all).

Map<?, ?> data1Map isn't untyped. A raw Map would be untyped. But, (Map<String, Object>) is unchecked. The ? removes the bad unchecked operations while keeping the compiler checks. (Map<?, ?>) is a checked cast which is strictly better than an unchecked cast, all without losing information. The @SuppressWarnings is for suppressing when we can't get rid of the unchecked cast.

I tried to follow best practices of Effective Java https://www.informit.com/articles/article.aspx?p=2861454&seqNum=2.

Eliminate every unchecked warning that you can. If you eliminate all warnings, you are assured that your code is typesafe, which is a very good thing. It means that you won’t get a ClassCastException at runtime, and it increases your confidence that your program will behave as you intended.

@rnc rnc Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changing to I think is simply saying we have no ideas about type so there are no possible warnings to throw. This means then any .get operations aren't checked versus the original.

@dwalluck dwalluck Aug 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since the (Map<String, Object>) data1 is an unchecked cast it can never add a check since that's what unchecked means.

It can do nothing except possibly raise a ClassCastException. Here,get's key is always Object and V is Object, so they are both just Object.

The only feature it's adding is a theoretical exception and no check. That's why I quoted Effective Java before.

@rnc rnc Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Full disclosure - Bob helped here!

You're right that (Map<String, Object>) is an unchecked cast in the specification sense. However, looking at the actual usage on lines 170–172 and 189–190, there are no per-value casts here either — the .get() results go directly into assertEquals(Object, Object). So the "cast the get, not the Map" principle from Effective Java isn't what's being applied; the values simply don't need casting at all in this context.

The honest justification here is the comment on line 168 itself: "if we do not know the type, access Map directly". When the code genuinely doesn't know or care about the type parameters, Map is the right declaration — it accurately reflects that intent. That is a sound reason on its own, without needing the Effective Java argument about unchecked casts.

So I'm ok with this one after further analysis.

assertEquals( 48475, data1Map.get( "package_id" ) );
assertEquals( 513598, data1Map.get( "build_id" ) );
assertEquals( "org.dashbuilder-dashbuilder-parent-metadata", data1Map.get( "package_name" ) );


// b. verify response from listTags call

List<Object> data2List = (List<Object>) data2;
List<?> data2List = (List<?>) data2;
assertEquals( 4, data2List.size() );

// if we know the type (KojiTagInfo) in the list, parse the element to it
Expand All @@ -186,7 +186,7 @@ private void assertMultiCallResponse( MultiCallResponse response )
// if we do not know the type, access List directly
Object data2_1 = data2List.get( 0 );
assertTrue( data2_1 instanceof Map );
Map<String, Object> data2_1Map = (Map<String, Object>) data2_1;
Map<?, ?> data2_1Map = (Map<?, ?>) data2_1;
assertEquals( "jb-bxms-6.3-candidate", data2_1Map.get( "name" ) );
assertEquals( 8829, data2_1Map.get( "id" ) );
}
Expand Down
5 changes: 3 additions & 2 deletions rwx/src/main/java/org/commonjava/rwx/core/Registry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/**
/*
* Copyright (C) 2010 Red Hat, Inc. (http://github.com/Commonjava/commonjava)
*
* Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -49,7 +49,8 @@ public <T> T parseAs( Object o, Class<T> type )

public Object renderTo( Object obj )
{
Renderer renderer = rendererMap.get( obj.getClass() );
@SuppressWarnings( "unchecked" )
Renderer<Object> renderer = (Renderer<Object>) rendererMap.get( obj.getClass() );
if ( renderer == null )
{
throw new IllegalArgumentException( "Renderer not found for " + obj.getClass() );
Expand Down
11 changes: 9 additions & 2 deletions rwx/src/main/java/org/commonjava/rwx/util/ParseUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/**
/*
* Copyright (C) 2010 Red Hat, Inc. (http://github.com/Commonjava/commonjava)
*
* Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -61,7 +61,14 @@ public static Object upgradeCast( Class<?> clazz, Object value )

private static <T> Class<T> wrap( Class<T> c )
{
return c.isPrimitive() ? (Class<T>) PRIMITIVES_TO_WRAPPERS.get( c ) : c;
if ( !c.isPrimitive() )
{
return c;
}

@SuppressWarnings( "unchecked" )
Class<T> wrapped = (Class<T>) PRIMITIVES_TO_WRAPPERS.get( c );
return wrapped;
}

private static final Map<Class<?>, Class<?>> PRIMITIVES_TO_WRAPPERS = new HashMap<>();
Expand Down
20 changes: 10 additions & 10 deletions rwx/src/main/java/org/commonjava/rwx/util/RenderUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,19 +61,19 @@ else if ( rpcObject instanceof MethodResponse )
}
else if ( rpcObject instanceof Map<?, ?> )
{
return toStructPartXMLString( (Map<String, Object>) rpcObject );
return toStructPartXMLString( (Map<?, ?>) rpcObject );
}
else if ( rpcObject instanceof List )
{
return toArrayPartXMLString( (List<Object>) rpcObject );
return toArrayPartXMLString( (List<?>) rpcObject );
}
else
{
throw new XmlRpcException( "Not supported, " + rpcObject.getClass() );
}
}

private static String toStructPartXMLString( Map<String, Object> rpcObject ) throws XmlRpcException
private static String toStructPartXMLString( Map<?, ?> rpcObject ) throws XmlRpcException
{
StringWriter result = new StringWriter();
try
Expand All @@ -89,7 +89,7 @@ private static String toStructPartXMLString( Map<String, Object> rpcObject ) thr
return result.toString();
}

private static String toArrayPartXMLString( List<Object> rpcObject ) throws XmlRpcException
private static String toArrayPartXMLString( List<?> rpcObject ) throws XmlRpcException
{
StringWriter result = new StringWriter();
try
Expand Down Expand Up @@ -173,11 +173,11 @@ private static void writeValue( XMLStreamWriter w, Object object ) throws XMLStr

if ( object instanceof List )
{
writeArray( w, (List<Object>) object );
writeArray( w, (List<?>) object );
}
else if ( object instanceof Map<?, ?> )
{
writeStruct( w, (Map<String, Object>) object );
writeStruct( w, (Map<?, ?>) object );
}
else
{
Expand All @@ -187,7 +187,7 @@ else if ( object instanceof Map<?, ?> )
w.writeEndElement();
}

private static void writeArray( XMLStreamWriter w, List<Object> objects )
private static void writeArray( XMLStreamWriter w, List<?> objects )
throws XMLStreamException, CoercionException
{
w.writeStartElement( ARRAY );
Expand All @@ -200,15 +200,15 @@ private static void writeArray( XMLStreamWriter w, List<Object> objects )
w.writeEndElement();
}

private static void writeStruct( XMLStreamWriter w, Map<String, Object> map )
private static void writeStruct( XMLStreamWriter w, Map<?, ?> map )
throws XMLStreamException, CoercionException
{
w.writeStartElement( STRUCT );
for ( Map.Entry<String, Object> entry : map.entrySet() )
for ( Map.Entry<?, ?> entry : map.entrySet() )
{
w.writeStartElement( MEMBER );
w.writeStartElement( NAME );
w.writeCharacters( entry.getKey() );
w.writeCharacters( (String) entry.getKey() );
w.writeEndElement();
writeValue( w, entry.getValue() );
w.writeEndElement();
Expand Down
Loading
Loading