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

Add support for GraalVM #1582

Closed
wants to merge 2 commits into from
Closed

Conversation

ilopmar
Copy link

@ilopmar ilopmar commented Dec 10, 2020

Fixes #1552


name: Pull Request
about: Create a report to help us improve
title: ''
labels: Status:Discovery
assignees: ''


Environment

Liquibase Version: 4.2.0+

Liquibase Integration & Version: all

Liquibase Extension(s) & Version:

Database Vendor & Version: all

Operating System Type & Version:

Pull Request Type

  • Bug fix (non-breaking change which fixes an issue.)
  • Enhancement/New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

A clear and concise description of the issue being addressed. Additional guidance here.

  • Describe the actual problematic behavior.
  • Ensure private information is redacted.

Steps To Reproduce

Follow up on #1552. This PR is a first step to add support for GraalVM out of the box in Liquibase so anyone using it can benefit. This avoids that every application, framework and library need to include the specific support.

Actual Behavior

It is not possible to create a Java application that uses Liquibase and works with GraalVM out of the box.

Expected/Desired Behavior

It is possible to create a GraalVM native image of a Java application that uses Liquibase and runs the migrations when the application starts.

Additional Context

The included files in this PR is what I needed to do in Micronaut to make it compatible with Liquibase. As mentioned in #1552 I don't know the internals of this library so you will probably need to adjust the configuration (maybe adding or removing some classes or resources).

Fast Track PR Acceptance Checklist:

Need Help?

Come chat with us on our discord channel

@codecov
Copy link

codecov bot commented Dec 10, 2020

Codecov Report

Merging #1582 (18c8688) into master (232b4a4) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1582   +/-   ##
=========================================
  Coverage     48.21%   48.22%           
  Complexity     7941     7941           
=========================================
  Files           794      794           
  Lines         38582    38582           
  Branches       6919     6919           
=========================================
+ Hits          18604    18605    +1     
  Misses        17508    17508           
+ Partials       2470     2469    -1     
Impacted Files Coverage Δ Complexity Δ
...e/src/main/java/liquibase/util/DependencyUtil.java 92.10% <0.00%> (+0.87%) 0.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 232b4a4...18c8688. Read the comment docs.

@molivasdat molivasdat added DBAll ImpactLow IntegrationAny RiskMedium Changes that require more testing or that affect many different code paths. Severity4 TypeEnhancement labels Dec 11, 2020
@molivasdat
Copy link
Contributor

Hi @ilopmar
Thanks for this enhancement request
Here’s what happens next:

A member of the Liquibase team will take a look at your contribution and may suggest:

The PR will be prioritized according to our internal development and testing capacity.

We’ll let you know when it’s ready to move to the next step or if any changes are needed.

@molivasdat molivasdat added this to To Do in Conditioning++ Jun 8, 2021
@ssssteffff
Copy link

Have you made any progress on this integration? We just struggled with this problem and gave up on using includeAll because of this.

@molivasdat
Copy link
Contributor

We are trying to slot this and add this to a version cycle. No updates yet.

@nvoxland nvoxland moved this from To Do to In discussion in Conditioning++ Jun 28, 2021
@nvoxland
Copy link
Contributor

The setup in this PR is a good start to GraalVM config, but there is more needed and we are not yet ready to commit to managing the needed GraalVM config long-term.

I merged this PR (as well as a few updates) into a new https://github.com/liquibase/liquibase/tree/graalvm-support branch which we will accept PRs into but will be a "use at your own risk" branch until we understand the demand more.

I'll update the original issue with more details.

@nvoxland nvoxland closed this Jul 26, 2021
Conditioning++ automation moved this from In discussion to Done Jul 26, 2021
@nvoxland nvoxland removed this from Done in Conditioning++ Jul 28, 2021
@sdeleuze
Copy link

The next step could be trying to run some Liquibase tests using https://github.com/graalvm/native-build-tools. Mockito is not supported but regular JUnit 5 tests are.

@mraible
Copy link

mraible commented Sep 29, 2021

@joshlong and I discovered today that adding these files to a project isn't enough when you're using the Spring integration. Error is:

2021-09-29 14:08:01.404  WARN 31832 --- [           main] onfigReactiveWebServerApplicationContext : 
Exception encountered during context initialization - 
cancelling refresh attempt: org.springframework.beans.factory.BeanCreationException: 
Error creating bean with name 'liquibase' defined in class path resource 
[com/mycompany/myapp/config/LiquibaseConfiguration.class]: 
Bean instantiation via factory method failed; nested exception is org.springframework.beans.BeanInstantiationException: 
Failed to instantiate [liquibase.integration.spring.SpringLiquibase]: Factory method 'liquibase' threw exception; nested 
exception is liquibase.exception.UnexpectedLiquibaseException: 
java.lang.NoSuchMethodException: liquibase.configuration.LiquibaseConfiguration.<init>()
...

Native reflection configuration for liquibase.configuration.LiquibaseConfiguration.<init>() is missing.

@mraible
Copy link

mraible commented Sep 29, 2021

We were able to fix things by using the JSON files + adding type hints for a few classes.

@TypeHint(
    types = {
        liquibase.configuration.LiquibaseConfiguration.class,
        com.zaxxer.hikari.HikariDataSource.class,
        liquibase.change.core.LoadDataColumnConfig.class
    },
    typeNames = {
        "com.zaxxer.hikari.util.ConcurrentBag$IConcurrentBagEntry[]"
    },
    access = AccessBits.ALL)

@ilopmar
Copy link
Author

ilopmar commented Sep 30, 2021

Right, you need to add those classes if using Hikari. I didn't include them here and even mentioned because those are already included in Micronaut: https://github.com/micronaut-projects/micronaut-sql/blob/master/jdbc-hikari/src/main/resources/META-INF/native-image/io.micronaut.sql/micronaut-jdbc-hikari/reflect-config.json

@mraible
Copy link

mraible commented Sep 30, 2021

I'm using Spring Boot in a JHipster app.

@nvoxland
Copy link
Contributor

I am working on test cleanup including trying to get away from Mockito in favor of the Spock test mocks. Do you know if those work with GraalVM, @sdeleuze?

@nvoxland
Copy link
Contributor

Right, you need to add those classes if using Hikari. I didn't include them here and even mentioned because those are already included in Micronaut: https://github.com/micronaut-projects/micronaut-sql/blob/master/jdbc-hikari/src/main/resources/META-INF/native-image/io.micronaut.sql/micronaut-jdbc-hikari/reflect-config.json

That reflect-config.json doesn't list liquibase.configuration.LiquibaseConfiguration or liquibase.change.core.LoadDataColumnConfig.class. Is it the java.sql.Statement line that makes those classes work?

Wondering what needs to be added to the liquibase branch to make it work out of the box outside micronaut

@joshlong
Copy link

@mraible and I took that reflect-config.json and added the following to get an app that builds/runs out of the box with Spring Native. It would be wonderful if these files were merged into the trunk and released, please. They're not much good to anyone sitting in this branch 9+ months later...


 {
    "name": "com.zaxxer.hikari.HikariConfig",
    "allDeclaredMethods": true,
    "allDeclaredFields": true,
    "allDeclaredConstructors": true
  },
  {
    "name": "java.util.concurrent.CopyOnWriteArrayList",
    "allDeclaredMethods": true,
    "allDeclaredFields": true,
    "allDeclaredConstructors": true
  },
  {
    "name": "java.sql.Statement[]",
    "allDeclaredMethods": true,
    "allDeclaredFields": true,
    "allDeclaredConstructors": true
  },
  {
    "name": "com.zaxxer.hikari.HikariDataSource",
    "allDeclaredMethods": true,
    "allDeclaredFields": true,
    "allDeclaredConstructors": true
  },
  {
    "name": "liquibase.configuration.LiquibaseConfiguration",
    "allDeclaredMethods": true,
    "allDeclaredFields": true,
    "allDeclaredConstructors": true
  },
  {
    "name": "com.zaxxer.hikari.util.ConcurrentBag.IConcurrentBagEntry[]",
    "allDeclaredMethods": true,
    "allDeclaredFields": true,
    "allDeclaredConstructors": true
  },
  {
    "name": "liquibase.change.core.LoadDataColumnConfig",
    "allDeclaredMethods": true,
    "allDeclaredFields": true,
    "allDeclaredConstructors": true
  }

@sdeleuze
Copy link

sdeleuze commented Oct 1, 2021

@nvoxland I don't think Spoke test mocks will be better on native due to Groovy, but maybe we could setup some kind of integration tests which would be good enough to test the native configuration.

Also I would like to know more about the kind of reflection required for Liquibase on those classes since GraalVM 21.3 is going to introduce new capabilities, like the ability to differentiate reflection metadata (cheap) from reflective invocation (costly). What kind of reflection is required for Liquibase?

It is strange to discuss this on a closed issue, could we reopen it?

@snicoll
Copy link

snicoll commented Oct 2, 2021

@sdeleuze this is a PR, the related issue is open. Perhaps the discussion should happen there instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DBAll ImpactLow IntegrationAny RiskMedium Changes that require more testing or that affect many different code paths. Severity4 TypeEnhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for GraalVM native images
9 participants