search

amadeus4dev / amadeus-java

Java library for the Amadeus Self-Service travel APIs
https://developers.amadeus.com/
MIT License
87 stars 69 forks source link

Add compilation support for Java 8 & Java 11 #184

Closed steve-donovan closed 2 years ago

steve-donovan commented 2 years ago

Description

When having a later JDK installed >8, the amadeus-java tests fail.

Steps to Reproduce

  1. install a version of JDK > 9
  2. run the tests ./gradlew test

Expected Behavior: all tests from master branch to pass

Actual Behavior: almost half of the tests fail

Stable Behavior? 100%

Versions

JDK 15 Mac O/S 12.4

Cause

The issue is the removal of unsafe operations from JDK 9 onward.

Solution

update the version of mockito-core (simplest) or add the following to build.gradle file (obviously replacing my path with an env variable). User experience would suffer with the latter solution as they'd need to install another JRE and set up an env variable.

tasks.withType(Test) {
    executable = new File("/Library/Internet Plug-Ins/JavaAppletPlugin.plugin/Contents/Home/", 'bin/java')
}

should be more like:

tasks.withType(Test) {
    executable = new File("${PATH_TO_TEST_JRE}", 'bin/java')
}
steve-donovan commented 2 years ago

I'm happy to make the changes once someone decides approach

jabrena commented 2 years ago

Hi Steven,

let’s review the dependency to upgrade it. testImplementation "org.mockito:mockito-core:2.12.0"

https://mvnrepository.com/artifact/org.mockito/mockito-core

Juan Antonio

steve-donovan commented 2 years ago

Hi Steven,

let’s review the dependency to upgrade it. testImplementation "org.mockito:mockito-core:2.12.0"

https://mvnrepository.com/artifact/org.mockito/mockito-core

Juan Antonio

all tests run and succeed with the latest version, and as it's mockito, its touch is confined to test suites

// https://mvnrepository.com/artifact/org.mockito/mockito-core
testImplementation 'org.mockito:mockito-core:4.6.1'
jabrena commented 2 years ago

Hi @steve-donovan,

I was reviewed again the issue, and you are using a non LTS JVM version (Java 15).

The last LTS JVM versions are:

Source: https://www.oracle.com/java/technologies/java-se-support-roadmap.html

Did you test with Java 17 the solution that you mention?

steve-donovan commented 2 years ago

Hi @jabrena

I had not tried 17 - I'll try doing so shortly and will let you know the result.

steve-donovan commented 2 years ago

Hi @jabrena

Beyond JDK 15 you get a Lombok error. I'm looking into it now

jabrena commented 2 years ago

Maybe, it is related with this issue: https://github.com/projectlombok/lombok/issues/3219

and it is necessary to wait for a new version to improve the support for Java 17.

steve-donovan commented 2 years ago

Maybe, it is related with this issue: projectlombok/lombok#3219

Maybe, it is necessary a new version for Java 17.

yeah, it was in a way, and that issue helped a lot. I actually solved it for lombok but now have a single test failing com.amadeus.travel.TripParserIT#given_client_when_call_trip_parser_with_params_alternative_1_then_ok

class com.amadeus.travel.TripParser (in unnamed module @0x5a2e4553) cannot access class com.sun.org.apache.xerces.internal.impl.dv.util.Base64 (in module java.xml) because module java.xml does not export com.sun.org.apache.xerces.internal.impl.dv.util to unnamed module @0x5a2e4553

I'm investigating now.

steve-donovan commented 2 years ago

Hi @jabrena

the breaking test is due to a call to internal api String b64Encoded = Base64.encode(Files.readAllBytes(file.toPath()));

issue described here [adoptium/adoptium-support/issues/199]

jabrena commented 2 years ago

It is great:

I actually solved it for lombok but now have a single test failing

How did you do it?

The Base64 issue was registered here: https://github.com/amadeus4dev/amadeus-java/issues/139

In next months, we want to increase the minimum JVM version supported to Java 8, and later Java 11, so we could use the native Base64 support: https://docs.oracle.com/javase/8/docs/api/java/util/Base64.html

steve-donovan commented 2 years ago

bumper mockito version to solve original issue, then added/amended scoped lines for bumped version of lombok

    compileOnly "org.projectlombok:lombok:1.18.24"
    annotationProcessor 'org.projectlombok:lombok:1.18.24'
    testCompileOnly 'org.projectlombok:lombok:1.18.24'
    testAnnotationProcessor 'org.projectlombok:lombok:1.18.24'
    testImplementation 'org.mockito:mockito-core:4.6.1'
steve-donovan commented 2 years ago

ok - got it build.gradle change

dependencies {
    implementation 'com.google.code.gson:gson:2.9.0'
    implementation 'jakarta.xml.bind:jakarta.xml.bind-api:4.0.0'
    compileOnly "org.projectlombok:lombok:1.18.24"
    annotationProcessor 'org.projectlombok:lombok:1.18.24'
    testCompileOnly 'org.projectlombok:lombok:1.18.24'
    testAnnotationProcessor 'org.projectlombok:lombok:1.18.24'
    testImplementation 'org.mockito:mockito-core:4.6.1'
    testRuntimeOnly "org.slf4j:slf4j-api:1.7.10"
    testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.1'
    testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.8.1'
    testImplementation 'org.assertj:assertj-core:3.22.0'
    testImplementation 'com.github.tomakehurst:wiremock:2.27.2'
    testImplementation 'io.rest-assured:rest-assured:5.0.0'
    testImplementation 'org.slf4j:slf4j-api:1.7.36'
    testImplementation 'org.slf4j:slf4j-simple:1.7.36'
}

and TripParser change:

  public TripDetail post(File file) throws ResponseException, IOException {
    // Base64 encode file and create request body
    String b64Encoded = DatatypeConverter.printBase64Binary(Files.readAllBytes(file.toPath()));

    JsonObject body = new JsonObject();
    body.addProperty("payload", b64Encoded);

    Response response = client.post("/v3/travel/trip-parser", body);
    return (TripDetail) Resource.fromObject(response, TripDetail.class);
  }

I can raise a PR if you like 😊

jabrena commented 2 years ago

Give me the opportunity to think about it.

I believe, that it is not possible to do it in a single round everything:

We could update the pipeline build_gradle.yml in this way to compile for multiple JVM versions with no change in the build system: https://github.com/actions/setup-java

jobs:
  build:
    runs-on: ubuntu-20.04
    strategy:
      matrix:
        java: [ '8', '11' ]
    name: Java ${{ matrix.Java }} sample
    steps:
      - uses: actions/checkout@v3
      - name: Setup java
        uses: actions/setup-java@v3
        with:
          distribution: 'adopt'
          java-version: ${{ matrix.java }}

If you observe, in one round, we could increase the compilation support for Java 8 & Java 11 but not for Java 17 immediately, because it is necessary to increase the minimum compilation support for Java 8 (We are discussing internally). In your side, using sdk cli tool, you could switch to different JVM versions in one second, for example from java 17 to java 19 (next september) or to java 11 or to java 8. :)

If you like, we could receive a PR for actions 1 & 3 in this round.

Juan Antonio