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

Backporting #40 to ognl-3-1-x branch. #41

Merged
merged 1 commit into from Nov 16, 2017

Conversation

harawata
Copy link
Contributor

When scanning super interfaces in OgnlRuntime#getMethods(Class, boolean), only default methods should be collected instead of all declared methods.
Also, super interfaces should be searched recursively so that it can handle deeper interface hierarchy.

When scanning super interfaces in `OgnlRuntime#getMethods(Class, boolean)`, only default methods should be collected instead of all declared methods.
Also, super interfaces should be searched recursively so that it can handle deeper interface hierarchy.
@lukaszlenart
Copy link
Collaborator

LGTM 💯 & thank you :)

@lukaszlenart lukaszlenart merged commit 212e142 into orphan-oss:ognl-3-1-x Nov 16, 2017
@lukaszlenart
Copy link
Collaborator

OGNL 3.1.16 is out, could you test it and report any problems?

@lukaszlenart
Copy link
Collaborator

Tested with Struts 2.6-SNAPSHOT on Java 8 and works :)

@harawata
Copy link
Contributor Author

Thank you, @lukaszlenart !
I'll test it later tonight.

@harawata
Copy link
Contributor Author

@lukaszlenart
I have tested 3.1.16 with MyBatis and it seems to be working fine. Thanks again!

@lukaszlenart
Copy link
Collaborator

@harawata osm! Thank you so much for your contribution :)

@YSavanier
Copy link

YSavanier commented Sep 27, 2018

Hello, I'm sorry but I think the PR was missing important diffs since code added by harawata ( I'm a big fan of mybatipse by the way thank you for your hard work :D ) on class OgnlRuntime line 2150 to the getDeclaredMethods method included an interface default methods scanning routine which is missing of current master branch hence breaking unit Java8Test testGetDeclaredMethods as reported in issue 51

Could you please consider adding the mssing code in next minor release please as for today we were forced to ovveride the OgnlRuntime class to add the missing code :

// Add default methods on interface
for (Class c = targetClass; c != null; c = c.getSuperclass()) {
for(Class intf : c.getInterfaces()){
Method[] methods = intf.getDeclaredMethods();

    for (int i = 0; i < methods.length; i++) {
        if (Modifier.isAbstract(methods[i].getModifiers()))
            continue;

        String ms = methods[i].getName();

        if (ms.endsWith(baseName)) {
            boolean isSet = false, isIs = false;

            if ((isSet = ms.startsWith(SET_PREFIX)) || ms.startsWith(GET_PREFIX)
                || (isIs = ms.startsWith(IS_PREFIX))) {
                int prefixLength = (isIs ? 2 : 3);

                if (isSet == findSets) {
                    if (baseName.length() == (ms.length() - prefixLength)) {
                        if (result == null) {
                            result = new ArrayList();
                        }
                        result.add(methods[i]);
                    }
                }
            }
        }
    }
}

}

Thank you very much to both of you.

@YSavanier YSavanier mentioned this pull request Sep 27, 2018
@harawata
Copy link
Contributor Author

Hi @YSavanier ,

That is one of the 'not-directly-related things' that I mentioned in #40 .
I ignored it because it didn't affect MyBatis :P
Anyway, I'll see what I can do.

And I'm glad you found the plugin useful! :)

@YSavanier
Copy link

Yeah I didn't encountered the problem in MyBatis (for which by the way I'm also glad you took the lead after eduardo macaron took a break from it), I was using the engine on a JOpenDocument stack to generate documents using embeded expression language so it used another path to fill the cacheGetMethod map.

@harawata
Copy link
Contributor Author

Eduardo is the lead. He's just being busy right now. =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants