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

Updates to OGNL 3.2.x OgnlRuntime #64

Merged
merged 1 commit into from
Nov 26, 2018
Merged

Updates to OGNL 3.2.x OgnlRuntime #64

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
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
<dependency>
<groupId>org.javassist</groupId>
<artifactId>javassist</artifactId>
<version>3.24.0-GA</version>
<version>3.23.1-GA</version>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Uh... I missed that out :\

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @lukaszlenart .

The only reason I noticed it was I ran the Maven build with JDK7 (and it failed due to "Unsupported major.minor version 52.0" related to Javassist). :)

It seems like they jumped from source/target 1.6 right to 1.8 in 3.24.0-GA (the commit that made the change didn't explain why ...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @lukaszlenart .

In addition to the JDK version support levels for Javassist 3.24 vs. 3.23, I ran across a note of possible issues with 3.23.x (jboss-javassist/javassist#224). It's possible that falling back to Javassist 3.22 for OGNL 3.2.x, or opening an issue report with Javassist and asking for restoration of JDK 7 support in 3.25+ might be necessary ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does OGNL even need javassist anymore? I'm not sure if anyone ever used those features other than me in Tapestry 4. It might be safe to just drop that feature set instead. Up to you. =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @jkuhnert .

Not sure if anyone out there needs Javassist for OGNL. If neither you nor @lukaszlenart are aware of any active Javassist feature usage, maybe it could marked for deprecation in the OGNL 3.2.x line (or simply phased out in future 3.2.x and noted in the release notes) ?

I was just trying to keep it compiling on JDK 7. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's used in ExpressionCompiler and I don't know if this can be easily replaced

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @lukaszlenart .

Based on your comment about ExpressionCompiler and a brief look at the code, it certainly looks like Javassist is pretty fundamental to its design...

As a "heads-up" the Javassist team accepted a PR (jboss-javassist/javassist#230) to restore JDK 7 runtime compatibility. That could mean that the next Javassist 3.24.x or 3.25 release might work for the OGNL 3.2.x line, whenever the next Javassist official build is released.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great news, thanks a lot!!!

</dependency>
</dependencies>

Expand Down
74 changes: 49 additions & 25 deletions src/main/java/ognl/OgnlRuntime.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,8 +870,11 @@ public static Object invokeMethod(Object target, Method method, Object[] argsArr
synchronized(method)
{
((AccessibleObject) method).setAccessible(true);
result = method.invoke(target, argsArray);
((AccessibleObject) method).setAccessible(false);
try {
result = method.invoke(target, argsArray);
} finally {
((AccessibleObject) method).setAccessible(false);
}
}
} else
{
Expand Down Expand Up @@ -1956,7 +1959,7 @@ public static Object getFieldValue(OgnlContext context, Object target, String pr
throws NoSuchFieldException
{
Object result = null;
Field f = getField((target == null) ? null : target.getClass(), propertyName);
final Field f = getField((target == null) ? null : target.getClass(), propertyName);

if (checkAccessAndExistence) {
if ((f == null) || !context.getMemberAccess().isAccessible(context, target, f, propertyName)) {
Expand All @@ -1968,14 +1971,17 @@ public static Object getFieldValue(OgnlContext context, Object target, String pr
throw new NoSuchFieldException(propertyName);
} else {
try {
Object state = null;

if (!Modifier.isStatic(f.getModifiers())) {
state = context.getMemberAccess().setup(context, target, f, propertyName);
result = f.get(target);
context.getMemberAccess().restore(context, target, f, propertyName, state);
} else
final Object state = context.getMemberAccess().setup(context, target, f, propertyName);
try {
result = f.get(target);
} finally {
context.getMemberAccess().restore(context, target, f, propertyName, state);
}
} else {
throw new NoSuchFieldException(propertyName);
}

} catch (IllegalAccessException ex) {
throw new NoSuchFieldException(propertyName);
Expand All @@ -1991,19 +1997,21 @@ public static boolean setFieldValue(OgnlContext context, Object target, String p
boolean result = false;

try {
Field f = getField((target == null) ? null : target.getClass(), propertyName);
Object state;

if ((f != null) && !Modifier.isStatic(f.getModifiers())) {
state = context.getMemberAccess().setup(context, target, f, propertyName);
try {
if (isTypeCompatible(value, f.getType())
|| ((value = getConvertedType(context, target, f, propertyName, value, f.getType())) != null)) {
f.set(target, value);
result = true;
final Field f = getField((target == null) ? null : target.getClass(), propertyName);

if (f != null) {
final int fModifiers = f.getModifiers();
if (!Modifier.isStatic(f.getModifiers()) && !Modifier.isFinal(fModifiers)) {
final Object state = context.getMemberAccess().setup(context, target, f, propertyName);
try {
if (isTypeCompatible(value, f.getType())
|| ((value = getConvertedType(context, target, f, propertyName, value, f.getType())) != null)) {
f.set(target, value);
result = true;
}
} finally {
context.getMemberAccess().restore(context, target, f, propertyName, state);
}
} finally {
context.getMemberAccess().restore(context, target, f, propertyName, state);
}
}
} catch (IllegalAccessException ex) {
Expand All @@ -2029,15 +2037,30 @@ public static boolean hasField(OgnlContext context, Object target, Class inClass
return (f != null) && isFieldAccessible(context, target, f, propertyName);
}

/**
* Method name is getStaticField(), but actually behaves more like "getStaticFieldValue()".
* <p/>
* Typical usage: Returns the value (not the actual {@link Field}) for the given (static) fieldName.
* May return the {@link Enum} constant value for the given fieldName when className is an {@link Enum}.
* May return a {@link Class} instance when the given fieldName is "class".
* <p/>
* @param context The current ognl context
* @param className The name of the class which contains the field
* @param fieldName The name of the field whose value should be returned
*
* @return The value of the (static) fieldName
* @throws OgnlException
*/
public static Object getStaticField(OgnlContext context, String className, String fieldName)
throws OgnlException
{
Exception reason = null;
try {
Class c = classForName(context, className);
final Class c = classForName(context, className);

if (c == null)
if (c == null) {
throw new OgnlException("Unable to find class " + className + " when resolving field name of " + fieldName);
}

/*
* Check for virtual static field "class"; this cannot interfere with normal static
Expand All @@ -2055,16 +2078,17 @@ public static Object getStaticField(OgnlContext context, String className, Strin
}
}

Field f = getField(c, fieldName);
final Field f = getField(c, fieldName);
if (f == null) {
throw new NoSuchFieldException(fieldName);
}
if (!Modifier.isStatic(f.getModifiers()))
if (!Modifier.isStatic(f.getModifiers())) {
throw new OgnlException("Field " + fieldName + " of class " + className + " is not static");
}

Object result = null;
if (context.getMemberAccess().isAccessible(context, null, f, null)) {
Object state = context.getMemberAccess().setup(context, null, f, null);
final Object state = context.getMemberAccess().setup(context, null, f, null);
try {
result = f.get(null);
} finally {
Expand Down
Loading