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.

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

Allow for subtype resolution with unknown generics #53

Open
CarstenWickner opened this issue Mar 2, 2020 · 7 comments
Open

Allow for subtype resolution with unknown generics #53

CarstenWickner opened this issue Mar 2, 2020 · 7 comments
Labels
has-failing-test There is a reproduction (inlined, unit test, or external) of the issue

Comments

@CarstenWickner
Copy link

CarstenWickner commented Mar 2, 2020

I've been using classmate for type resolutions within my victools/jsonschema-generator library.

Unfortunately, I've hit a snag. It's this particular piece of code in the TypeResolver (lines 288-299):

for (int i = 0; i < paramCount; ++i) {
    ResolvedType t = placeholders[i].actualType();
    // Is it ok for it to be left unassigned? For now let's not allow that
    // 18-Oct-2017, tatu: Highly likely that we'll need to allow this, substitute with "unknown" --
    //    had to do that in Jackson. Occurs when subtype is generic, with "bogus" type declared
    //    but not bound in supertype(s). But leaving checking in for now.
    if (t == null) {
        throw new IllegalArgumentException("Failed to find type parameter #"+(i+1)+"/"
                +paramCount+" for "+subtype.getName());
    }
    typeParams[i] = t;
}

Short term: I'm wondering what exactly I can do on my side short of copy-pasting the whole resolveSubtype() method and leaving out this unfortunate exception being thrown?
Can I somehow provide extra type bindings (even if they are just Object.class) to make it work?

Mid term: could there be some kind of configuration option on the type resolver to simply set it to (the resolved version of) Object.class or some lower bounds of the generic (if that's not already happening) instead of throwing an exception?
How did you handle this in Jackson (as per the above in-line comment)?

@cowtowncoder
Copy link
Member

I think addition of configuration option would be the best option here?
Very short-term: if you can figure out a refactoring that would allow simple sub-classing of relevant component (so you can override it), I'd be happy to merge a PR.

In Jackson I think a simple "unknown type" (~= Object.class) is just quietly added in such cases.

@cowtowncoder cowtowncoder added good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project hacktoberfest Issue related to Hactoberfest2020 activities, eligible for additional rewards labels Oct 13, 2020
@takanuva15
Copy link

I know this is a bit late, but I've been running into an issue where I have some classes that implement Comparator but a few don't provide the generic type parameter (The code is legacy from another jar which I don't have the ability to fix currently.)

So when I go to look up the classes implementing Comparator within my package, it throws an IndexOutOfBoundsException in this method within TypeResolver.java:

    private void _resolveTypePlaceholders(ResolvedType sourceType, ResolvedType actualType) throws IllegalArgumentException {
        List<ResolvedType> expectedTypes = sourceType.getTypeParameters();
        List<ResolvedType> actualTypes = actualType.getTypeParameters(); //exception here because no Comparator type specified
        int i = 0;

        for(int len = expectedTypes.size(); i < len; ++i) {
            ResolvedType exp = (ResolvedType)expectedTypes.get(i);
            ResolvedType act = (ResolvedType)actualTypes.get(i);
            if (!this._verifyAndResolve(exp, act)) {
                throw new IllegalArgumentException("Type parameter #" + (i + 1) + "/" + len + " differs; expected " + exp.getBriefDescription() + ", got " + act.getBriefDescription());
            }
        }

    }

By default, Comparator has a generic parameter of Object, but my model class has no generic type, so this line throws the exception because there is no actualType to get: ResolvedType act = (ResolvedType)actualTypes.get(i);

Does this issue match what you guys are talking about here?

@cowtowncoder
Copy link
Member

@takanuva15 Would it be possible to include type declarations needed for reproduction? Definitely there should never be ArrayIndexOutOfBoundsException and it might be an easy enough fix.
Not sure if this is the same problem as reported here originally but sounds worth fixing regardless.

@takanuva15
Copy link

It was my own custom class implementing Comparator like class MyComparator implements Comparator, but there was no type specified for the generic parameter of Comparator (it was legacy code). I got rid of the issue by moving the Comparators to a different package so it wasn't included in the subtype lookup code for the package I was operating on. In any case, if it comes up again as a blocker I'll try to post more specific code.

@cowtowncoder
Copy link
Member

Thank you @takanuva15 ! Maybe I can reproduce it easily with that.

@cowtowncoder
Copy link
Member

@takanuva15 Ok, so I can not quite reproduce exception with type like:

    @SuppressWarnings("rawtypes")
    static abstract class Comparator53 implements Comparator { }

but I do see a problem, since this:

        ResolvedType rt = typeResolver.resolve(Comparator53.class);
        List<ResolvedType> params = rt.typeParametersFor(Comparator.class);

returns an empty List, instead of one with 1 entry (for java.lang.Object).
So that at least seems wrong. Although not sure quite easy it'll be to fix.

But I would also like to see the actual failure you see; whether it is due to calling some other method(s) on ResolvedType, or if type declaration is different.

cowtowncoder added a commit that referenced this issue Nov 16, 2023
@cowtowncoder cowtowncoder added has-failing-test There is a reproduction (inlined, unit test, or external) of the issue and removed hacktoberfest Issue related to Hactoberfest2020 activities, eligible for additional rewards good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project labels Nov 16, 2023
@cowtowncoder
Copy link
Member

Added a failing case for problem I did find. Still hoping to get another reproduction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-failing-test There is a reproduction (inlined, unit test, or external) of the issue
Projects
None yet
Development

No branches or pull requests

3 participants