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

Add Exception Type Parsing #1031

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

Add Exception Type Parsing #1031

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
6 changes: 6 additions & 0 deletions class-model/src/main/java/org/glassfish/hk2/classmodel/reflect/MethodModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ public interface MethodModel extends Member, AnnotatedElement {
*/
String[] getArgumentTypes();

/**
* @return the checked exception types, or an empty array if the method doesn't
* declare any thrown exceptions
*/
String[] getExceptionTypes();

/**
* Returns the list of parameter
*
Expand Down
19 changes: 19 additions & 0 deletions class-model/src/main/java/org/glassfish/hk2/classmodel/reflect/impl/MethodModelImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
public class MethodModelImpl extends AnnotatedElementImpl implements MethodModel {

private List<Parameter> parameters;
private List<ParameterizedType> exceptionTypes;
private ParameterizedType returnType;
final ExtensibleType<?> owner;
private final String signature;
Expand Down Expand Up @@ -74,6 +75,24 @@ public String[] getArgumentTypes() {
return stringTypes;
}

@Override
public String[] getExceptionTypes() {
String[] stringTypes;
if (exceptionTypes != null) {
stringTypes = new String[exceptionTypes.size()];
for (int i = 0; i < exceptionTypes.size(); i++) {
stringTypes[i] = exceptionTypes.get(i).getTypeName();
}
} else {
stringTypes = new String[0];
}
return stringTypes;
}

public void setExceptionTypes(List<ParameterizedType> exceptionTypes) {
this.exceptionTypes = exceptionTypes;
}

@Override
public List<Parameter> getParameters() {
return parameters;
Expand Down
10 changes: 10 additions & 0 deletions ...l/src/main/java/org/glassfish/hk2/classmodel/reflect/impl/MethodSignatureVisitorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public class MethodSignatureVisitorImpl extends SignatureVisitor {
private final MethodModel methodModel;

private final List<Parameter> parameters = new ArrayList<>();
private final List<ParameterizedType> exceptionTypes = new ArrayList<>();
private final ParameterizedType returnType = new ParameterizedTypeImpl();
private final ArrayDeque<ParameterizedType> parentType = new ArrayDeque<>();

Expand All @@ -50,6 +51,10 @@ public List<Parameter> getParameters() {
return parameters;
}

public List<ParameterizedType> getExceptionTypes() {
return exceptionTypes;
}

public ParameterizedType getReturnType() {
return returnType;
}
Expand All @@ -62,6 +67,11 @@ public SignatureVisitor visitParameterType() {
return this;
}

@Override
public SignatureVisitor visitExceptionType() {
return this;
Copy link
Contributor

@pzygielo pzygielo May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't exceptionTypes be updated somewhere, perhaps here? Otherwise - how it gets populated?

Edit: I've removed locally this method and collection+getter and the test is fine with such change. Do we need exceptionTypes in this class?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @pzygielo. This code doesn't do anything right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will review - I was simply cherry-picking from our fork (I didn't make the original change)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an inherited method from the SignatureVisitor class from ASM: SignatureVisitor#L153.

There is an identical implementation of it over in the SignatureVisitorImpl class here in HK2: SignatureVisitorImpl#L88.

So I assume the original intent was to simply mirror the behaviour of the SignatureVisitorImpl class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be to follow established pattern, right.
What would be the role of exceptionTypes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would need to spend some time digging into it with a debugger - it's a bit abstract for my eyes to dry-run through.

The interface method from MethodModel (and presumably MethodModelImpl) gets used over in Payara for MicroProfile OpenAPI processing here and here.

Within HK2 itself the important changes within this PR seem to be working on the MethodModel within the ModelClassVisitor (here). This code is preceded by reading the method signature (which is where this MethodSignatureVisitorImpl class comes into play), during which at least one code path seems to "visit" the exception type (which is where I start getting lost without a debugger).

Copy link
Contributor

Choose a reason for hiding this comment

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

@Pandrex247, list exceptionTypes at L39 always stay empty because it never updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

list exceptionTypes at L39 always stay empty because it never updated.

Probably we don't know. getExceptionTypes returns original, mutable list so this MIGHT be updated anywhere, out of control of the class.
I understand getParameters does the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand getParameters does the same.

But parameters collection is filled in the visitParameterType() method at L63 of the MethodSignatureVisitorImpl class.

Actual exception types processing occurs in the ModelClassVisitor class beginnin with L284.

mutable list so this MIGHT be updated anywhere, out of control of the class.

In this case even out of control HK2.

}

@Override
public SignatureVisitor visitReturnType() {
parentType.add(returnType);
Expand Down
11 changes: 11 additions & 0 deletions class-model/src/main/java/org/glassfish/hk2/classmodel/reflect/impl/ModelClassVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,17 @@ public MethodVisitor visitMethod(int access, String name, String desc, String si
org.objectweb.asm.Type type = org.objectweb.asm.Type.getReturnType(desc);
returnType.setType(type);

final List<ParameterizedType> exceptionTypes = new ArrayList<>();
if (exceptions != null) {
for (int i = 0; i < exceptions.length; i++) {
final String exception = exceptions[i];
final ParameterizedTypeImpl exceptionType = new ParameterizedTypeImpl(exception);
exceptionType.setType(org.objectweb.asm.Type.getObjectType(exception));
exceptionTypes.add(exceptionType);
}
}
methodModel.setExceptionTypes(exceptionTypes);

org.objectweb.asm.Type[] types = org.objectweb.asm.Type.getArgumentTypes(desc);
for (int i = 0; i < methodModel.getParameters().size(); i++) {
ParameterImpl parameter = (ParameterImpl) methodModel.getParameter(i);
Expand Down
5 changes: 5 additions & 0 deletions class-model/src/test/java/org/glassfish/hk2/classmodel/reflect/test/method/MethodTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@ public void simpleTest() throws IOException, InterruptedException {
Assert.assertEquals("yellow", gradientColor2.get(0).getValues().get("name"));
Assert.assertEquals("orange", gradientColor2.get(1).getValues().get("name"));

// Exception types
final String[] exceptionTypes = mm.getExceptionTypes();
Assert.assertEquals(1, exceptionTypes.length);
Assert.assertEquals(IllegalArgumentException.class.getName(), exceptionTypes[0]);

// Parameter annotations, type and generic types check
Assert.assertEquals(5, mm.getParameters().size());

Expand Down
2 changes: 1 addition & 1 deletion ...src/test/java/org/glassfish/hk2/classmodel/reflect/test/method/SimpleAnnotatedMethod.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public SampleType<Integer, Character, Boolean> setFoo(
List<String> input,
SampleType<Double, String, SampleType<Short, Float, Long>> sampleType,
int count,
Object value) {
Object value) throws IllegalArgumentException {
return null;
}
}
Expand Down