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

FieldValueResolver leads to IllegalAccessException in 4.3.0 #940

Open
mdesharnais opened this issue Jan 22, 2022 · 23 comments
Open

FieldValueResolver leads to IllegalAccessException in 4.3.0 #940

mdesharnais opened this issue Jan 22, 2022 · 23 comments
Labels

Comments

@mdesharnais
Copy link
Contributor

mdesharnais commented Jan 22, 2022

Hi,

the following code, wich uses the FieldValueResolver, leads to an InaccessibleObjectException.

import com.github.jknack.handlebars.Context;
import com.github.jknack.handlebars.Handlebars;
import com.github.jknack.handlebars.context.FieldValueResolver;
import com.github.jknack.handlebars.io.FileTemplateLoader;

public final class Main {
  public static void main(final String[] args) throws Throwable {
    final var loader = new FileTemplateLoader(".", ".hbs");
    final var handlebars = new Handlebars(loader);

    final String str = handlebars.compile("foo").apply(
      Context.newBuilder(null).resolver(FieldValueResolver.INSTANCE).build());

    System.out.println(str);
  }
}

Assuming that this code is in the file Main.java and that, sitting next to it, the file foo.hbs contains the following.

AAA{{> bar}}CCC

And that the file bar.hbs contains the following.

BBB

Then the following command produces the exception.

$ java -version
openjdk version "17.0.1" 2021-10-19
OpenJDK Runtime Environment (build 17.0.1+12-Debian-1deb11u2)
OpenJDK 64-Bit Server VM (build 17.0.1+12-Debian-1deb11u2, mixed mode, sharing)
$ java -cp "./handlebars-4.3.0.jar:./slf4j-api-1.7.32.jar" Main.java                                   
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".                                                                                              
SLF4J: Defaulting to no-operation (NOP) logger implementation                                                                                                 
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.                                                                            
Exception in thread "main" com.github.jknack.handlebars.HandlebarsException: ./foo.hbs:1:7: java.lang.IllegalStateException: Shouldn't be illegal to access field 'size'                                                                                                                                                    
    ./foo.hbs:1:7                                                                                                                                             
        at com.github.jknack.handlebars.context.FieldValueResolver.invokeMember(FieldValueResolver.java:217)                                                  
        at com.github.jknack.handlebars.context.FieldValueResolver.invokeMember(FieldValueResolver.java:39)                                                   
        at com.github.jknack.handlebars.context.MemberValueResolver.resolve(MemberValueResolver.java:59)                                                      
        at com.github.jknack.handlebars.Context$CompositeValueResolver.resolve(Context.java:200)                                                              
        at com.github.jknack.handlebars.internal.path.PropertyPath.eval(PropertyPath.java:52)                                                                 
        at com.github.jknack.handlebars.Context$PathExpressionChain.next(Context.java:361)                                                                    
        at com.github.jknack.handlebars.Context$PathExpressionChain.eval(Context.java:381)                                                                    
        at com.github.jknack.handlebars.Context.get(Context.java:621)                                                                                         
        at com.github.jknack.handlebars.internal.Partial.override(Partial.java:253)                                                                           
        at com.github.jknack.handlebars.internal.Partial.merge(Partial.java:226)                                                                              
        at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:126)                                                                    
        at com.github.jknack.handlebars.internal.TemplateList.merge(TemplateList.java:95)                                                                     
        at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:126)                                                                    
        at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:114)                                                                    
        at com.github.jknack.handlebars.internal.ForwardingTemplate.apply(ForwardingTemplate.java:100)                                                        
        at Main.main(Main.java:11)
Caused by: java.lang.IllegalStateException: Shouldn't be illegal to access field 'size'                                                                       
        at com.github.jknack.handlebars.context.FieldValueResolver.invokeMember(FieldValueResolver.java:217)                                                  
        at com.github.jknack.handlebars.context.FieldValueResolver.invokeMember(FieldValueResolver.java:39)                                                   
        at com.github.jknack.handlebars.context.MemberValueResolver.resolve(MemberValueResolver.java:59)                                                      
        at com.github.jknack.handlebars.Context$CompositeValueResolver.resolve(Context.java:200)                                                              
        at com.github.jknack.handlebars.internal.path.PropertyPath.eval(PropertyPath.java:52)                                                                 
        at com.github.jknack.handlebars.Context$PathExpressionChain.next(Context.java:361)                                                                    
        at com.github.jknack.handlebars.Context$PathExpressionChain.eval(Context.java:381)                                                                    
        at com.github.jknack.handlebars.Context.get(Context.java:621)          
        at com.github.jknack.handlebars.internal.Partial.override(Partial.java:253)                                                                           
        at com.github.jknack.handlebars.internal.Partial.merge(Partial.java:226)                                                                              
        at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:126)                                                                    
        at com.github.jknack.handlebars.internal.TemplateList.merge(TemplateList.java:95)                                  
        at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:126)
        at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:114)
        at com.github.jknack.handlebars.internal.ForwardingTemplate.apply(ForwardingTemplate.java:100)
        at Main.main(Main.java:11)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:568)
        at jdk.compiler/com.sun.tools.javac.launcher.Main.execute(Main.java:419)
        at jdk.compiler/com.sun.tools.javac.launcher.Main.run(Main.java:192)
        at jdk.compiler/com.sun.tools.javac.launcher.Main.main(Main.java:132)
Caused by: java.lang.IllegalAccessException: class com.github.jknack.handlebars.context.FieldValueResolver$FieldMembercannot access a member of class java.util.HashMap (in module java.base) with modifiers "transient"
        at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:392)
        at java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:674)
        at java.base/java.lang.reflect.Field.checkAccess(Field.java:1102)
        at java.base/java.lang.reflect.Field.get(Field.java:423)
        at com.github.jknack.handlebars.context.FieldValueResolver$FieldMember.get(FieldValueResolver.java:135)
        at com.github.jknack.handlebars.context.FieldValueResolver.invokeMember(FieldValueResolver.java:214)
        ... 22 more
mdesharnais added a commit to mdesharnais/handlebars.java that referenced this issue Feb 13, 2022
mdesharnais added a commit to mdesharnais/handlebars.java that referenced this issue Feb 13, 2022
@jhannes
Copy link

jhannes commented Feb 22, 2022

@mdesharnais I get this as well. The underlying problem seems to be that the access restrictions are stronger in Java 17. I resolved it by overriding FieldResolver like so:

    @Override
    public boolean matches(FieldWrapper field, String name) {
        return !isStatic(field) && field.getName().equals(name) && isPublic(field);
    }

Presumably, this will be a problem for any templates that have assumed access to private fields.

@jknack
Copy link
Owner

jknack commented Feb 22, 2022

Guys,

You need to stop accessing private fields in Java 17. Field must be public or you need a getter method.

@jhannes
Copy link

jhannes commented Feb 22, 2022

Hey @jknack: This happens when you pass a map into handlebars. We're not the ones using private fields, you are ;-)

I tried the following, where example.txt.hbs just is a simple import so it copies the context:

        Handlebars handlebars = new Handlebars(new ClassPathTemplateLoader());
        Template tmpl = handlebars.compile("example.txt");

        System.out.println(tmpl.apply(Context
                .newBuilder(new HashMap<String, String>(Map.of()))
                .resolver(FieldValueResolver.INSTANCE)
                .build()));

The result is the same as @mdesharnais 's stack trace above.

@jknack
Copy link
Owner

jknack commented Feb 22, 2022

@jhannes Look here. It checks what Java version are you running and setup the value resolvers properly.

This work:

Handlebars handlebars = new Handlebars(new ClassPathTemplateLoader());
        Template tmpl = handlebars.compile("example.txt");

        System.out.println(tmpl.apply(Context
                .newBuilder(new HashMap<String, String>(Map.of()))
                .build()));

Because it uses the default value resolvers. Now if you override that and just add FieldValueResolver, then it won't work. If yo still need to access to public field then add it to the end:

Handlebars handlebars = new Handlebars(new ClassPathTemplateLoader());
        Template tmpl = handlebars.compile("example.txt");

        System.out.println(tmpl.apply(Context
                .newBuilder(new HashMap<String, String>(Map.of()))
                .resolver(MapValueResolver.INSTANCE,
          JavaBeanValueResolver.INSTANCE, MethodValueResolver.INSTANCE, FieldValueResolver.INSTANCE)
                .build()));

@jhannes
Copy link

jhannes commented Feb 22, 2022

@jknack Thanks for the clarification. This looks good. However, I can't get it to work properly in my context, which is OpenAPI Generator

I've tried overriding OpenAPI's adaptor and remove the call to .resolver(), but then it breaks majorly (lots of data is omitted). Overriding FieldResolver with the isPublic check works.

@jknack
Copy link
Owner

jknack commented Feb 22, 2022

Odd. We can probably add the public check on Java 14 or higher. But again, people need to understand this issue is part of upgrading to new JVM.

@jhannes
Copy link

jhannes commented Feb 22, 2022

I totally agree. Private fields are going away. But at the moment, there is no migration path for e.g. openapi-generator. I'll examine the behavior a bit more so I can create a better reproduction

@jhannes
Copy link

jhannes commented Feb 23, 2022

@jknack I have made some more research into the OpenAPI with Java 17 issue and have found an example that mimics the issue.

The following test fails to generate output:

class FixedHandlebarsEngineAdapterTest {

    public static class MyFieldValueResolver extends FieldValueResolver {

        @Override
        public boolean matches(FieldWrapper field, String name) {
            return super.matches(field, name) && !isTransient(field);
        }

        protected boolean isTransient(FieldWrapper member) {
            return Modifier.isTransient(member.getModifiers());
        }
    }

    @Test
    void java17failure() throws IOException {
        HashMap<Object, Object> bundle = exampleModel();

        Context context = Context
                .newBuilder(bundle)
                // Commenting in the following will make it work
                /*
                .resolver(
                        MapValueResolver.INSTANCE,
                        JavaBeanValueResolver.INSTANCE,
                        new MyFieldValueResolver())

                 */
                .build();

        Handlebars handlebars = new Handlebars();
        // Commenting out this will make it work again
        /*
         */
        handlebars.registerHelperMissing((obj, options) -> {
            System.err.printf(Locale.ROOT, "Unregistered helper name '%s', processing template:%n%s%n", options.helperName, options.fn.text());
            return "";
        });
        handlebars.getLoader().setPrefix("/typescript-fetch-api/");
        handlebars.getLoader().setSuffix(".handlebars");
        handlebars.registerHelper("json", Jackson2Helper.INSTANCE);
        StringHelpers.register(handlebars);
        handlebars.registerHelpers(ConditionalHelpers.class);
        handlebars.registerHelpers(org.openapitools.codegen.templating.handlebars.StringHelpers.class);
        Template tmpl = handlebars.compile("model");

        String output = tmpl.apply(context);
        System.out.println(output);
        assert !output.isBlank();
    }

    private HashMap<Object, Object> exampleModel() {
        HashMap<Object, Object> bundle = new HashMap<>();
        ArrayList<Object> models = new ArrayList<>();
        bundle.put("models", models);
        Map<String, Object> petDto = new HashMap<>();
        petDto.put("importPath", "PetDto");
        CodegenModel petModel = new CodegenModel();
        petModel.classname = "PetDto";
        petModel.parent = "AnimalDto";
        petModel.isEnum = false;
        CodegenProperty property = new CodegenProperty();
        property.name = "petName";
        property.dataType = "string";
        petModel.vars.add(property);
        petDto.put("model", petModel);
        models.add(petDto);
        return bundle;
    }
}

Given the following templates:

{{#models}}{{#model}}{{#isEnum}}{{>modelEnum}}{{/isEnum}}{{^isEnum}}{{>modelGeneric}}{{/isEnum}}{{/model}}{{/models}}
export interface {{classname}} {{#if parent}}extends {{{parent}}} {{/if}}{

{{~#vars}}
    {{~#if description}}
    /**
     * {{{description}}}
     */
    {{~/if}}
    {{#isReadOnly}}readonly {{/isReadOnly}}{{name}}{{^required}}?{{/required}}{{#required}}{{#isReadOnly}}?{{/isReadOnly}}{{/required}}: {{#if isEnum}}{{{datatypeWithEnum}}}{{else if isFile}}Blob{{else}}{{{dataType}}}{{/if}}{{#isNullable}} | null{{/isNullable}};
{{~/vars}}
}

The issue seems to be composed of several problems and I don't know why it doesn't work.

I'm wondering if Block.java is checking for it == null when it should be checking for helper == null.

As Handlebars use in OpenAPI Generator is an important use case for the project, I think this is could be an important issue

@mdesharnais
Copy link
Contributor Author

mdesharnais commented Mar 13, 2022

Hi @jknack @jhannes,
I tried to add the FieldValueResolver at the end but then encountered a new problematic situation when accessing an element from the outer context that happens to have the same name as an unexposed field of the class behind the current context.

import java.util.Map;
import java.util.List;

import com.github.jknack.handlebars.Context;
import com.github.jknack.handlebars.Handlebars;
import com.github.jknack.handlebars.context.FieldValueResolver;
import com.github.jknack.handlebars.context.JavaBeanValueResolver;
import com.github.jknack.handlebars.context.MapValueResolver;
import com.github.jknack.handlebars.context.MethodValueResolver;

public final class Main {
  public static void main(final String[] args) throws Throwable {
    final var handlebars = new Handlebars();

    final String str = handlebars
      .compileInline("{{value}}{{#strings}} {{value}}.{{.}}{{/strings}}")
      .apply(
        Context.newBuilder(
          Map.of(
            "value", "aaa",
            "strings", List.of("bbb", "ccc")))
          .resolver(
            MapValueResolver.INSTANCE,
            JavaBeanValueResolver.INSTANCE,
            MethodValueResolver.INSTANCE,
            FieldValueResolver.INSTANCE)
          .build());

    System.out.println(str);
  }
}

I would expect the output to be "aaa aaa.bbb aaa.ccc", but get the same IllegalAccessException again. Apparently, the String class has an internal field with the same name as an object of my Handlebars context.

$ java -version
openjdk version "17.0.2" 2022-01-18
OpenJDK Runtime Environment (build 17.0.2+8-Debian-1deb11u1)
OpenJDK 64-Bit Server VM (build 17.0.2+8-Debian-1deb11u1, mixed mode, sharing)
$ java -cp "./handlebars-4.3.0.jar:./slf4j-api-1.7.32.jar" Main.java
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
Exception in thread "main" com.github.jknack.handlebars.HandlebarsException: inline@7fe0618c:1:24: java.lang.IllegalStateException: Shouldn't be illegal to access field 'value'
    inline@7fe0618c:1:24
	at com.github.jknack.handlebars.context.FieldValueResolver.invokeMember(FieldValueResolver.java:217)
	at com.github.jknack.handlebars.context.FieldValueResolver.invokeMember(FieldValueResolver.java:39)
	at com.github.jknack.handlebars.context.MemberValueResolver.resolve(MemberValueResolver.java:59)
	at com.github.jknack.handlebars.Context$CompositeValueResolver.resolve(Context.java:200)
	at com.github.jknack.handlebars.internal.path.PropertyPath.eval(PropertyPath.java:52)
	at com.github.jknack.handlebars.Context$PathExpressionChain.next(Context.java:361)
	at com.github.jknack.handlebars.Context$PathExpressionChain.eval(Context.java:381)
	at com.github.jknack.handlebars.Context.get(Context.java:618)
	at com.github.jknack.handlebars.internal.Variable.value(Variable.java:169)
	at com.github.jknack.handlebars.internal.Variable.merge(Variable.java:146)
	at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:126)
	at com.github.jknack.handlebars.internal.TemplateList.merge(TemplateList.java:95)
	at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:126)
	at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:114)
	at com.github.jknack.handlebars.Options.apply(Options.java:538)
	at com.github.jknack.handlebars.helper.EachHelper.apply(EachHelper.java:73)
	at com.github.jknack.handlebars.internal.Block.merge(Block.java:211)
	at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:126)
	at com.github.jknack.handlebars.internal.TemplateList.merge(TemplateList.java:95)
	at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:126)
	at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:114)
	at com.github.jknack.handlebars.internal.ForwardingTemplate.apply(ForwardingTemplate.java:100)
	at Main.main(Main.java:17)
Caused by: java.lang.IllegalStateException: Shouldn't be illegal to access field 'value'
	at com.github.jknack.handlebars.context.FieldValueResolver.invokeMember(FieldValueResolver.java:217)
	at com.github.jknack.handlebars.context.FieldValueResolver.invokeMember(FieldValueResolver.java:39)
	at com.github.jknack.handlebars.context.MemberValueResolver.resolve(MemberValueResolver.java:59)
	at com.github.jknack.handlebars.Context$CompositeValueResolver.resolve(Context.java:200)
	at com.github.jknack.handlebars.internal.path.PropertyPath.eval(PropertyPath.java:52)
	at com.github.jknack.handlebars.Context$PathExpressionChain.next(Context.java:361)
	at com.github.jknack.handlebars.Context$PathExpressionChain.eval(Context.java:381)
	at com.github.jknack.handlebars.Context.get(Context.java:618)
	at com.github.jknack.handlebars.internal.Variable.value(Variable.java:169)
	at com.github.jknack.handlebars.internal.Variable.merge(Variable.java:146)
	at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:126)
	at com.github.jknack.handlebars.internal.TemplateList.merge(TemplateList.java:95)
	at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:126)
	at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:114)
	at com.github.jknack.handlebars.Options.apply(Options.java:538)
	at com.github.jknack.handlebars.helper.EachHelper.apply(EachHelper.java:73)
	at com.github.jknack.handlebars.internal.Block.merge(Block.java:211)
	at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:126)
	at com.github.jknack.handlebars.internal.TemplateList.merge(TemplateList.java:95)
	at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:126)
	at com.github.jknack.handlebars.internal.BaseTemplate.apply(BaseTemplate.java:114)
	at com.github.jknack.handlebars.internal.ForwardingTemplate.apply(ForwardingTemplate.java:100)
	at Main.main(Main.java:17)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at jdk.compiler/com.sun.tools.javac.launcher.Main.execute(Main.java:419)
	at jdk.compiler/com.sun.tools.javac.launcher.Main.run(Main.java:192)
	at jdk.compiler/com.sun.tools.javac.launcher.Main.main(Main.java:132)
Caused by: java.lang.IllegalAccessException: class com.github.jknack.handlebars.context.FieldValueResolver$FieldMember cannot access a member of class java.lang.String (in module java.base) with modifiers "private final"
	at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:392)
	at java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:674)
	at java.base/java.lang.reflect.Field.checkAccess(Field.java:1102)
	at java.base/java.lang.reflect.Field.get(Field.java:423)
	at com.github.jknack.handlebars.context.FieldValueResolver$FieldMember.get(FieldValueResolver.java:135)
	at com.github.jknack.handlebars.context.FieldValueResolver.invokeMember(FieldValueResolver.java:214)
	... 29 more

Considering that there is no way to know or control the fields unexposed by third party code, I argue that FieldValueResolver should treat illegal access as if the field did not exist.

@jhannes
Copy link

jhannes commented Mar 15, 2022

Considering that there is no way to know or control the fields unexposed by third party code, I argue that FieldValueResolver should treat illegal access as if the field did not exist.

I second @mdesharnais 's proposal here

@jknack
Copy link
Owner

jknack commented Mar 15, 2022

Field must be public in modern JVM. You can see the root cause here:

Caused by: java.lang.IllegalAccessException: class com.github.jknack.handlebars.context.FieldValueResolver$FieldMember cannot access a member of class java.lang.String (in module java.base) with modifiers "private final"
	at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:392)

That is the JVM throwing the error, it isn't something the handlebars.java does. If the field doesn't exist, handlebars.java skip the resolver and execute next on the chain/pipeline.

Are you asking to ignore existing non-public fields on modern JVM?

@mdesharnais
Copy link
Contributor Author

Sorry for the delayed answer. Yes, I am asking for the FieldValueResolver to ignore non-public fields.

@agentgt
Copy link
Contributor

agentgt commented Mar 30, 2022

@jknack we probably could do something like the same trick we did here:
a7adb10#diff-9fd22b23d4adba304bdb22b65fe8094ba8b0f45943cb330a672b1dbaf52e5d97

Basically ignore field access to any class that is in the "java." or "sun." package including possibly even public ones (because in a modular world even if they are public they may not be open).

Otherwise we would have to use the modules API which would not work for JDK 8 compile.

@agentgt
Copy link
Contributor

agentgt commented Mar 30, 2022

The other option is to include modern value resolvers that are module aware in a separate maven module (not java module but maven) and compile them with JDK 11/17 (E.g. handlebars-jdk17-value-resolvers).

Then in the JDK 8 code base you could use the Service Loader to load them up (to avoid using weird reflection hacks or check java version). This would require a new SPI.

@agentgt
Copy link
Contributor

agentgt commented Mar 30, 2022

Sorry for the delayed answer. Yes, I am asking for the FieldValueResolver to ignore non-public fields.

Just so we are clear... that will not always work. I field can be public but still not accessible. You need to see if the class is public as well as the module is open for reflection.

Anyway jknack chose the correct behavior on not including the FieldValueResolver as a default precisely because of these problems. With records now ideally one should not use field value resolver for struct objects anyway (albeit I get backwards compatibility).

@mdesharnais
Copy link
Contributor Author

Hi @agentgt, thanks for looking into that.

Sorry for the delayed answer. Yes, I am asking for the FieldValueResolver to ignore non-public fields.

Just so we are clear... that will not always work. I field can be public but still not accessible. You need to see if the class is public as well as the module is open for reflection.

You are correct, I should have written, that I am asking for the FieldValueResolver to ignore fields not part of the object's published API, which should take public/non-public view as well as module view in consideration.

@agentgt
Copy link
Contributor

agentgt commented Mar 30, 2022

OK I see the issue. The cache still has members that are not accessible.

  private Map<String, M> cache(final Class<?> clazz) {
    Map<String, M> mcache = this.cache.get(clazz);
    if (mcache == null) {
      mcache = new HashMap<>();
      Set<M> members = members(clazz);
      for (M m : members) {
        // Mark as accessible.
        if (isUseSetAccessible(m) && m instanceof AccessibleObject) {
          ((AccessibleObject) m).setAccessible(true);
        }
////// WE are still adding the member
        mcache.put(memberName(m), m);
      }
      this.cache.put(clazz, mcache);
    }
    return mcache;
  }

One of the reasons I was looking into this is I made some of the reflection changes and the above is sort of a bug.

Anyway @jknack It looks we first need to check if its a setAccessibleObject. If it is not then we add it to the cache. If it is then check if isUseSetAccessible and if that is true setAccessible and then add it to the cache.

  private Map<String, M> cache(final Class<?> clazz) {
    Map<String, M> mcache = this.cache.get(clazz);
    if (mcache == null) {
      mcache = new HashMap<>();
      Set<M> members = members(clazz);
      for (M m : members) {
        // Mark as accessible.
        if (m instanceof AccessibleObject) {
          if (isUseSetAccessible(m)) {
            ((AccessibleObject) m).setAccessible(true);
            mcache.put(memberName(m), m);
          }
        }
        else {
          mcache.put(memberName(m), m);
        }
      }
      this.cache.put(clazz, mcache);
    }
    return mcache;
  }

@agentgt
Copy link
Contributor

agentgt commented Mar 31, 2022

Anyway a work around is this Field Value Resolver:

         var MY_FIELD_VALUE_RESOLVER = new FieldValueResolver() {

            @Override
            protected Set<FieldWrapper> members(
                    Class<?> clazz) {
                var members = super.members(clazz);
                return members.stream()
                    .filter(fw -> isValidField(fw))
                    .collect(Collectors.toSet());
            }

            boolean isValidField(
                    FieldWrapper fw) {
                if (fw instanceof AccessibleObject) {
                    if (isUseSetAccessible(fw)) {
                        return true;
                    }
                    return false;
                }
                return true;
            }
        };

You will need to java 8 if thats your target.

@agentgt
Copy link
Contributor

agentgt commented Apr 6, 2022

As I mentioned here #948 (comment)

We really cannot change FieldValueResolver otherwise folks will get very bizarre behavior on upgrades to newer JDKs without any errors (ie parent contexts now taking over illegal access fields).

And they will file bugs like #951 that are very hard to reproduce without their complete object graph.

The real solution is too deprecate FieldValueResolver in a minor version release. Then in a major version we rename FieldValueResolver to LegacyFieldValueResolver forcing folks code to break which is easier to fix and understand than templates breaking.

We then add another module to this project compiling FieldValueResolver with Java 9 or whatever using trySetAccessible. That module is an optional module (by module I mean maven module) that folks can add as a dependency.

@sribalajivelan-icanio
Copy link

@jhannes Look here. It checks what Java version are you running and setup the value resolvers properly.

This work:

Handlebars handlebars = new Handlebars(new ClassPathTemplateLoader());
        Template tmpl = handlebars.compile("example.txt");

        System.out.println(tmpl.apply(Context
                .newBuilder(new HashMap<String, String>(Map.of()))
                .build()));

Because it uses the default value resolvers. Now if you override that and just add FieldValueResolver, then it won't work. If yo still need to access to public field then add it to the end:

Handlebars handlebars = new Handlebars(new ClassPathTemplateLoader());
        Template tmpl = handlebars.compile("example.txt");

        System.out.println(tmpl.apply(Context
                .newBuilder(new HashMap<String, String>(Map.of()))
                .resolver(MapValueResolver.INSTANCE,
          JavaBeanValueResolver.INSTANCE, MethodValueResolver.INSTANCE, FieldValueResolver.INSTANCE)
                .build()));

Thanks for your help. It's working now.

@jhannes
Copy link

jhannes commented Jan 15, 2023

Is this included in 4.3.1? I still get "com.github.jknack.handlebars.HandlebarsException: baseApi.handlebars:4:3: java.lang.IllegalStateException: Shouldn't be illegal to access field 'size'"

@dobromyslov
Copy link

It looks like the fix has not been implemented yet.

I use plugin for Maven org.openapitools:openapi-generator-maven-plugin:7.7.0 and get almost the same exception:

Caused by: java.lang.IllegalStateException: Shouldn't be illegal to access field 'size'
at com.github.jknack.handlebars.context.FieldValueResolver.invokeMember (FieldValueResolver.java:217)
Caused by: java.lang.IllegalAccessException: class com.github.jknack.handlebars.context.FieldValueResolver$FieldMember cannot access a member of class java.util.HashMap (in module java.base) with modifiers "transient"
at com.github.jknack.handlebars.context.FieldValueResolver$FieldMember.get (FieldValueResolver.java:135)

The plugin is based on Handlebars 4.3.1 - https://github.com/OpenAPITools/openapi-generator/blob/ff2e173de8c68c304fa1cc1162a09aef44a34899/pom.xml#L1226

The proposed earlier FieldValueResolver is implemented in the current HandlebarsEngineAdapter - https://github.com/OpenAPITools/openapi-generator/blob/ff2e173de8c68c304fa1cc1162a09aef44a34899/modules/openapi-generator/src/main/java/org/openapitools/codegen/templating/HandlebarsEngineAdapter.java#L77

I wonder why HandlebarsEngineAdapter initializes FieldValueResolver before the MethodValueResolver:

// $ref: https://github.com/jknack/handlebars.java/issues/917
var MY_FIELD_VALUE_RESOLVER = new FieldValueResolver() {
    @Override
    protected Set<FieldWrapper> members(
            Class<?> clazz) {
        var members = super.members(clazz);
        return members.stream()
            .filter(fw -> isValidField(fw))
            .collect(Collectors.toSet());
    }

    boolean isValidField(
            FieldWrapper fw) {
        if (fw instanceof AccessibleObject) {
            if (isUseSetAccessible(fw)) {
                return true;
            }
            return false;
        }
        return true;
    }
};

Context context = Context
        .newBuilder(bundle)
        .resolver(
                MapValueResolver.INSTANCE,
                JavaBeanValueResolver.INSTANCE,
                MY_FIELD_VALUE_RESOLVER.INSTANCE,
                MethodValueResolver.INSTANCE)
        .build();

I'm gonna try to change the resolvers order, as @jknack said:

If you override that and just add FieldValueResolver, then it won't work. If yo still need to access to public field then add it to the end:

Handlebars handlebars = new Handlebars(new ClassPathTemplateLoader());
Template tmpl = handlebars.compile("example.txt");

    System.out.println(tmpl.apply(Context
            .newBuilder(new HashMap<String, String>(Map.of()))
            .resolver(MapValueResolver.INSTANCE,
      JavaBeanValueResolver.INSTANCE, MethodValueResolver.INSTANCE, FieldValueResolver.INSTANCE)
            .build()));

I would try to build a customized Handlebars adapter for OpenAPI Generator as described here https://openapi-generator.tech/docs/templating/#custom-engines

@WouterBaeyens
Copy link

WouterBaeyens commented Sep 23, 2024

@dobromyslov to avoid reasoning on wrong assumptions

The #940 (comment) is implemented in the current HandlebarsEngineAdapter

The proposed FieldValueResolver was so far never actually used (I opened a pull request to rectify the issue)

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

No branches or pull requests

7 participants