GeoIP2-java icon indicating copy to clipboard operation
GeoIP2-java copied to clipboard

Added `reflect-config.json` required to support graalvm native-image build.

Open bfg opened this issue 1 year ago • 3 comments

This PR, together with https://github.com/maxmind/MaxMind-DB-Reader-java/pull/166 adds full native-image support in geoip2-java.

Previously application, incorportaing geoip2-java threw exception when attempt was made to resolve some address:

com.maxmind.db.ConstructorNotFoundException: No constructor on class com.maxmind.geoip2.model.CityResponse with the MaxMindDbConstructor annotation was found.
        at com.maxmind.db.Decoder.findConstructor(Decoder.java:473)
        at com.maxmind.db.Decoder.decodeMapIntoObject(Decoder.java:394)
        at com.maxmind.db.Decoder.decodeMap(Decoder.java:341)
        at com.maxmind.db.Decoder.decodeByType(Decoder.java:162)
        at com.maxmind.db.Decoder.decode(Decoder.java:151)
        at com.maxmind.db.Decoder.decode(Decoder.java:76)
        at com.maxmind.db.Reader.resolveDataPointer(Reader.java:413)
        at com.maxmind.db.Reader.getRecord(Reader.java:185)
        at com.maxmind.geoip2.DatabaseReader.get(DatabaseReader.java:280)
        at com.maxmind.geoip2.DatabaseReader.getCity(DatabaseReader.java:365)
        at com.maxmind.geoip2.DatabaseReader.tryCity(DatabaseReader.java:359)

NOTE: This PR requires new release of maxmind/MaxMind-DB-Reader-java with native-image support.

bfg avatar Mar 29 '24 16:03 bfg

This config seems very likely to break as we add new classes. I see GraalVM issue on wildcard support, apparently filed by users of this library. It seems unlikely that will be added any time soon, but maybe there is a way to add a test to prevent future breakage.

Although I don't know much about native images, from the discussion on that issue, I assume it is possible to specify the config in the application including the library rather than including it here. Without an automated way to ensure that we do not accidentally break applications as we add new classes, I am inclined to hold off on merging this.

oschwald avatar Apr 01 '24 19:04 oschwald

hey @oschwald, you're right, this PR comes from my app that uses geoip2-java and gets compiled into native binary. Currently, the only 3rd library used in the app, that requires separate, custom reflect-config.json is geoip2-java 🤷; I wanted to improve the situation and developer UX and supplied a patch so that everyone can benefit from it.

And yes, sure, library maintainers need to continously maintain native-image references when they add/remove classes.

AWS SDK2 example:

  • https://github.com/search?q=repo%3Aaws%2Faws-sdk-java-v2+reflect-config.json&type=code
  • https://github.com/search?q=repo%3Aaws%2Faws-sdk-java-v2+native-image.properties&type=code
  • https://github.com/search?q=repo%3Aaws%2Faws-sdk-java-v2%20proxy-config.json&type=code

The best way to make sure that last commit didn't break native-image compatibility is by having a native-image test, see aws sdk2 example

bfg avatar Apr 02 '24 08:04 bfg

Thanks for the information. I think I would want a test to exist before merging this, as without it, we are unlikely to provide the kind of support that users of the library could rely on.

oschwald avatar Apr 02 '24 17:04 oschwald

@bfg I just had same exception, do you know are there any other library we can use which works with graalvm without this problem ? in my case I have a spring boot web app, are there any easy work around for this ?

ozkanpakdil avatar Dec 28 '24 10:12 ozkanpakdil

@ozkanpakdil hey, sorry for late reply, but in the end i wrote generic solution; I generate reflection-config.json for various SDKs as part of test-suite run, see

https://gist.github.com/bfg/afc9a512885cd511929096dc0c7067c2

bfg avatar Jan 29 '25 10:01 bfg