Add Exception Type Parsing

[Pandrex247]

Ports an old addition we've had in our Payara patched-src fork for a while.

Exception types are now parsed from MethodModels.
This is used within Payara to help with MicroProfile OpenAPI support.

[pzygielo]

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?

[avpinchuk]

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

[Pandrex247]

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

[Pandrex247]

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.

[pzygielo]

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

[Pandrex247]

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).

[avpinchuk]

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

[pzygielo]

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.

[avpinchuk]

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.

[OndroMih]

The current solution seems to work, the test passes. I debugged it now and found out that ASM doesn't pass info about exceptions in the signature argument to visitMethod of ModelClassVisitor. It only passes the info in the separate argument exceptions.

As a result, the method visitExceptionType here is never called. That's why it isn't enough to implement it.

However, I managed to get it working by manually calling the visitExceptionType() and then visitClassType methods on the visitor for each exception in the visitMethod class, right after the visitor is called by ASM signature reader. With this, things work as they should and the code is in line with the existing code, just with some help to the ASM SignatureReader, which doesn't find info about exceptions in the signature.

Here's are the changes: https://github.com/Pandrex247/glassfish-hk2/compare/Add-Exception-Type-Parsing...OndroMih:glassfish-hk2:ondromih-Add-Exception-Type-Parsing?expand=1

If it's OK like this, we can accept this PR and then I'll raise another PR with my improvements.

There are questions raised.

[OndroMih]

See the discussion about exceptionTypes ealier

[Pandrex247]

What is the requested change?

mutable list so this MIGHT be updated anywhere, out of control of the class.
In this case even out of control HK2.

Outside of the control of HK2, but the getter will still be required to allow this

[OndroMih]

@Pandrex247 , I think the concern is mainly coming from the complexity of the current implementation and ASM visitor API, and from the fact that the signature in visitMethod doesn't contain thrown exceptions, while the signature visitor in the ASM API contains a method visitExceptionType(), which is then never called. That's why the solution proposed by you had to work around it, confusing the reviewers.

As I have an improvement and I'm ready to submit it, I approved now and I can submit my improvement after this PR is merged. I'll just wait for a while until some of the other committers in the discussion have a look if it's OK.

[Pandrex247]

@OndroMih I'm happy to cherry-pick your change to this PR if you'd prefer to have it all in one?
I'm just running some tests against it in Payara now - initial results seem positive 😊

[OndroMih]

@Pandrex247 , sure, feel free to add my changes to your PR. Thanks!