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

reflect.json: allow for package wildcard my.package.* #1236

Open
vojkny opened this issue May 3, 2019 · 22 comments
Open

reflect.json: allow for package wildcard my.package.* #1236

vojkny opened this issue May 3, 2019 · 22 comments

Comments

@vojkny
Copy link

vojkny commented May 3, 2019

It would be very useful to be able to use wildcards for packages. Consider following reflect.json:

[  
    {  
        "name":"com.maxmind.geoip2.model.ConnectionTypeResponse",
        "allPublicMethods":true,
        "allDeclardConstructors":true
    },
    {  
        "name":"com.maxmind.geoip2.model.AsnResponse",
        "allPublicMethods":true,
        "allDeclardConstructors":true
    },
    {  
        "name":"com.maxmind.geoip2.model.CountryResponse",
        "allPublicMethods":true,
        "allDeclardConstructors":true
    },
    {  
        "name":"com.maxmind.geoip2.model.IspResponse",
        "allPublicMethods":true,
        "allDeclardConstructors":true
    },
    {  
        "name":"com.maxmind.geoip2.model.AbstractResponse",
        "allPublicMethods":true,
        "allDeclardConstructors":true
    },
    {  
        "name":"com.maxmind.geoip2.model.AbstractCountryResponse",
        "allPublicMethods":true,
        "allDeclardConstructors":true
    },
    {  
        "name":"com.maxmind.geoip2.model.CityResponse",
        "allPublicMethods":true,
        "allDeclardConstructors":true
    },
    {  
        "name":"com.maxmind.geoip2.model.EnterpriseResponse",
        "allPublicMethods":true,
        "allDeclardConstructors":true
    },
    {  
        "name":"com.maxmind.geoip2.model.DomainResponse",
        "allPublicMethods":true,
        "allDeclardConstructors":true
    },
    {  
        "name":"com.maxmind.geoip2.model.InsightsResponse",
        "allPublicMethods":true,
        "allDeclardConstructors":true
    },
    {  
        "name":"com.maxmind.geoip2.model.AnonymousIpResponse",
        "allPublicMethods":true,
        "allDeclardConstructors":true
    },
    {  
        "name":"com.maxmind.geoip2.model.AbstractCityResponse",
        "allPublicMethods":true,
        "allDeclardConstructors":true
    }
]

Which could just be replaced with:

[
    {  
        "name":"com.maxmind.geoip2.model.*",
        "allPublicMethods":true,
        "allDeclardConstructors":true
    }
]
@jroper
Copy link

jroper commented Aug 13, 2019

This would make using protobufs (and grpc) a lot easier, since protobufs use reflection to implement their equals/hashCode/toString methods, so every protobuf class generated must have all fields allowed for reflection. Generally, protobufs all get generated into their own package (or as inner classes of an outer class) so allowing wildcards for everything in a package, and for every inner class of an outerclass, would be very useful.

@taranion
Copy link

I could use that too. A am using a XML serializer, who makes use of reflection. I do have approx. 200 classes I would need to the JSON config (or Feature class) manually. Besides, having a static list configured for Graal is prone to fail, when the data model of the serialized classes is extended by getting added more classes.

@cyw3
Copy link
Contributor

cyw3 commented Dec 17, 2020

Any progress here?

@Legion2
Copy link

Legion2 commented Feb 9, 2021

I have a complex data model with over 100 classes, they are all in one package (and subpackages). It would be so easy if I could just add the whole package for reflection.

@MilosBogi
Copy link

I would agree that this feature is required.

@drm
Copy link

drm commented Sep 15, 2022

I would even argue that including reflection metadata for all included classes and their members should be the default and could easily provided by the compiler, I would think. Not including this is a form of premature optimization, imho. It's rather tedious now to convert an existing project to using Graal native-image because of this, while this is really not an optimization I want to worry about right now...

@davidreis97
Copy link

@drm I have to agree, this is a major issue for porting apps that heavily rely on reflection. I would love to hear from a maintainer about why this isn't the case by default, I'm sure there must be a good reason.

@amoscatelli
Copy link

bump ?

@srnagar
Copy link

srnagar commented Jan 23, 2023

Any update on this issue? We have many libraries each containing hundreds of models that are reflectively accessed by Jackson serializer. Enumerating every class used in serialization requires a lot of effort and results in a really large reflect-config.json file.

@prasannak-ra
Copy link

We've an exact same situation, when using hazelcast I had to add a long list of classes in reflection config, wildcard feature would really help.

@tlf30
Copy link

tlf30 commented May 29, 2023

We are working on a large swing application, and due to the way that swing property change listeners work we basically need every class in our app (hundreds of them) to have full reflection, along with several of our dependencies (also several hundred classes) if we could simply specify by package, or even just have a wildcard option that says ALL classes, this would be so much simpler.

@seroperson
Copy link

seroperson commented Dec 5, 2023

As a workaround, you can implement a custom Feature. I wrote an article about writing a custom Feature and there is also an example of such workaround (I used it to make htmlunit library work under GraalVM): https://seroperson.me/2023/12/05/implementing-a-graalvm-custom-feature/

@cstancu cstancu assigned loicottet and unassigned cstancu Dec 6, 2023
@neterium
Copy link

neterium commented Jan 10, 2024

Same issue here, there are hundreds of items in the JSON file, coming from all the dependencies we have. I'm really not comfortable with this. A good start would also be to let each dependency add its own file in META-INF and then collect/merge those files.

@vjovanov
Copy link
Member

We intend to implement this feature in the Gradle and Maven plugins. That should allow the end-user to make a library work and cover most use cases.

The reason for doing this in build tools is that wildcards in the reflect-config.json would foster unnecessary inclusion of code in the libraries leading to:

  1. Bloat in the images across the ecosystem.
  2. Extra complexity and confusion for the end user as frameworks would exclude excessive configuration to reduce image size.

By leaving the decision for mass inclusion to the end user the libraries in the ecosystem will always stay compact, but users can still easily make their projects work without a hustle.

Closing this ticket as all discussions should be continued at graalvm/native-build-tools#558.

@tlf30
Copy link

tlf30 commented Jan 12, 2024

@vjovanov the whole point of wildcards in the reflect config would be to include a large amount of code in the libraries, specifically for applications which require nearly everything to be accessible via reflection, such as swing applications that need to use java properties and bean bindings.

I do not believe giving the ability to have wildcards would add any confusion, especially considering the massive amount of existing confusion around how to get large reflection dependent applications to work.

Adding support I'm build tools to auto generate some limited form of a reflection config for the app is fine, but it not a substitute for wildcards, especially for large scale imports of libraries. This issue needs to reopened and properly addressed.

@srnagar
Copy link

srnagar commented Jan 12, 2024

@vjovanov Supporting this in build tool does not help library vendors. We don't control the build system of the end user consuming our libraries. We need a way to specify all the classes in our library in reflect config using a wildcard. The reflect config is packaged and shipped with our libraries.

Could you please reopen this issue and add support for specifying wildcards in reflect.json?

@vjovanov
Copy link
Member

I am reopening the issue to continue the discussion: I am fully aware of how difficult it can be to provide metadata in some cases. We need bulk inclusion of reflection and it needs to be easily usable by both library authors and end users.

Here is a list of issues I currently see with this proposal and ways how we could fix them:

  1. Making it easy to include all elements for reflection can have consequences for the image size and memory footprint of Native Images. The way around this is to only allow the inclusion of individual packages and not their sub-packages recursively. I will discuss this with people who have experience in this ecosystem to see their thoughts and whether it is safe to make this change.

  2. The reflect-config.json files have a scope across the whole classpath. This means that bulk inclusion in reflect-config.json can have accidental cross-library effects. For example, if any other library shares the same package suffix with the library that uses bulk inclusion we would have a situation where changes in the classpath can unexpectedly change program semantics. This might be unlikely to happen if we limit the scope and allow the inclusion of a single package per JSON entry.

  3. The reflection restriction is a security feature of Native Image. With the bulk inclusion, it becomes easier to include some elements by accident. I will consult our security team to see if this change would be acceptable.

  4. The proposed syntax is interesting but I find it can be used for strange queries. When people include individual method signatures (or fields) across a package it is unlikely that all classes in a package will have those methods. An alternative syntax could be :

    {
     "package": "com.maxmind.geoip2.model"
     "allDeclaredMethods": [true|false],
     "allDeclaredFields": [true|false], 
     "allDeclaredConstructors": [true|false],
     "allPublicMethods": [true|false],
     "allPublicFields": [true|false],
     "allPublicConstructors": [true|false]
    }
  5. The bulk inclusion would have trouble capturing lambdas as they are generated at runtime. This is doable by parsing all the classes in a package and instantiating lambdas.

@srnagar @tlf30 after stating all the design possibilities and restrictions, I can see that this can be an orthogonal feature to the implementation in the build tools. I will discuss this with our security team and with community representatives to see if we can move forward.

@vjovanov vjovanov reopened this Jan 15, 2024
@tlf30
Copy link

tlf30 commented Jan 15, 2024

@vjovanov Could this proposal also have a way to optionally include all sub-packages?
Such as:

{
 "package": "com.maxmind.geoip2.model"
 "allDeclaredMethods": [true|false],
 "allDeclaredFields": [true|false], 
 "allDeclaredConstructors": [true|false],
 "allPublicMethods": [true|false],
 "allPublicFields": [true|false],
 "allPublicConstructors": [true|false]
 "allSubPackages": [true|false]
}

This way for projects that have hundreds of package paths that need to be included it would be simple enough to just have a top level package entry in the reflection config for each library that is being used, or top level package that is being used.
In many cases, the included dependencies have already been vetted from a security perspective, and there are no concerns with having their classpaths included in reflection, since most apps that would run with a native image are already running in a standard JVM where full reflection is already present.

EDIT: A use case for this that would be especially important is when embedding a groovy scripting engine in an app, groovy uses almost pure reflection to perform any type of object access. This would require anything that groovy is accessing to be available via reflection, including the entire JDK.

@mikehearn
Copy link
Contributor

The problem with trying to make everything reflectable for the Groovy case is whether you still end up with improved performance, which is presumably the goal of using native image to start with. A big part of what makes native image generate efficient binaries is the highly restricted reflection. That's why there's some resistance in this case - it's easy to kill the goose that lays the golden eggs. If you want to run Groovy scripts that can access the entire JDK it's probable that a regular JVM is a better fit.

@tlf30
Copy link

tlf30 commented Jan 16, 2024

In my case, performance is not the concern for moving to native applications, instead it is being able to deploy applications in environments where a JVM is not allowed. There are many use cases for using graal outside of performance, and many of them come at performance costs compared to just running a JVM. Once example would be for embedded systems where a self-contained executable is required, graal is a great way to deploy a java application.

In other situations, the overall app may in-fact have large performance improvements from going to graal, but need to keep existing features (and those features may specifically have worse performance).

@Ivan1pl
Copy link

Ivan1pl commented Mar 13, 2024

Perhaps a less groundbreaking idea could be to introduce some kind of compact syntax? For example specifying multiple class names at once if their configuration is the same? Then the example provided by the issue author could be shortened to something like this:

[  
    {  
        "names": [
            "com.maxmind.geoip2.model.ConnectionTypeResponse",
            "com.maxmind.geoip2.model.AsnResponse",
            "com.maxmind.geoip2.model.CountryResponse",
            "com.maxmind.geoip2.model.IspResponse",
            "com.maxmind.geoip2.model.AbstractResponse",
            "com.maxmind.geoip2.model.AbstractCountryResponse",
            "com.maxmind.geoip2.model.CityResponse",
            "com.maxmind.geoip2.model.EnterpriseResponse",
            "com.maxmind.geoip2.model.DomainResponse",
            "com.maxmind.geoip2.model.InsightsResponse",
            "com.maxmind.geoip2.model.AnonymousIpResponse",
            "com.maxmind.geoip2.model.AbstractCityResponse"
        ],
        "allPublicMethods":true,
        "allDeclardConstructors":true
    }
]

@vjovanov
Copy link
Member

This is an interesting proposal. We intend to do this for the condition field.

We would need to figure out how this would work with #7476 and #7962. Also, interaction with tools like the agent and native-image configure is in question:

  • Should they fuse elements when they are the same? Probably not as random classes would get fused together
  • Should they keep the fused elements together when they process metadata? Probably yes, to keep things as users have written them.
    We will discuss this further and see if this is feasible. CC @loicottet

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

No branches or pull requests