Skip to content

Bootstrapper's intitialize method should be named initialize #25400

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

Closed
wants to merge 1 commit into from

Conversation

cprayer
Copy link
Contributor

@cprayer cprayer commented Feb 23, 2021

This PR fixes a minor typo in Bootstrapper class.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 23, 2021
@wilkinsona wilkinsona added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 23, 2021
@wilkinsona wilkinsona added this to the 2.4.x milestone Feb 23, 2021
@wilkinsona wilkinsona added the for: merge-with-amendments Needs some changes when we merge label Feb 23, 2021
@wilkinsona
Copy link
Member

wilkinsona commented Feb 23, 2021

Thanks for spotting this and opening a PR, @cprayer. We'll have to refine things a bit to deprecate the intitialize method and add the initialize method alongside it. Hopefully we can use a default method so that it's not a breaking change. We can do that as part of merging.

@wilkinsona wilkinsona changed the title Fix typo in Bootstrapper Bootstrapper's intitialize method should be named initialize Feb 23, 2021
@cprayer cprayer force-pushed the fix-typo branch 2 times, most recently from f32bfcb to a7367b4 Compare February 24, 2021 04:19
@cprayer
Copy link
Contributor Author

cprayer commented Feb 24, 2021

@wilkinsona Thank you for your review. I have changed the code as you said. How about this?

@cprayer cprayer force-pushed the fix-typo branch 2 times, most recently from 5bea2ec to 78297b9 Compare February 24, 2021 10:11
@philwebb
Copy link
Member

Oh dear, that's an embarrassing typo from me in the original 🤦 . Thanks for spotting it @cprayer.

@spencergibb
Copy link
Member

This doesn't handle the case where Bootstrapper implementers that has been released and only implemented intitialize (the misspelling) will continue to work. As it is coded, it will fail. I think that SpringApplication needs to continue to call the deprecated method until it is removed and the deprecated method needs to default to calling the new one.

@wilkinsona
Copy link
Member

Thanks for looking, @spencergibb.

I think that SpringApplication needs to continue to call the deprecated method until it is removed and the deprecated method needs to default to calling the new one.

I think it would be better if we could get into a situation where implementors don't have to have any code in the deprecated method.

Would it work if we make initialize (correct) a default method that calls intitialize (typo)? SpringApplication can then call initialize(correct) and the default method will delegate this to any existing implementation's intitialize (typo). When they upgrade and notice the deprecation warning, they can move the code from intitialize (typo) into initialize (correct) and leave intitialize (typo) empty as it'll never be called.

@philwebb
Copy link
Member

I wonder if we're going to have a binary compatibility issue if people are using the Bootstrapper as a lambda?

@philwebb
Copy link
Member

I'm guessing we might need to put the smarts in SpringApplication. We should probably call both methods and also deal with NoSuchMethodError.

@spencergibb
Copy link
Member

I'm fine either way, just as long as existing implementors don't get broken because they may use the deprecated method or have used a lambda.

@wilkinsona
Copy link
Member

I wonder if we're going to have a binary compatibility issue if people are using the Bootstrapper as a lambda?

I don't believe we will. Bootstrapper is still considered to be a functional interface when the default method is added as it still has a single abstract method.

We should probably call both methods and also deal with NoSuchMethodError.

That feels risky to me as, depending on how someone has implemented their Bootstrapper, it may result in initialization being performed twice.

I think we can do this with a default method as I outlined above.

@cprayer
Copy link
Contributor Author

cprayer commented Feb 28, 2021

@wilkinsona
I have wrote a simple code to make sure binary compatibility issue.
It is using openjdk 1.8.0_252, intellij idea community 2021.3.1, and gradle.

First version (https://github.com/cprayer/binary-compatibility-test-first-version)

Main.java

package org.example;

public class Main {
    public static void main(String[] args) {
        Typo typo = () -> "hello world";
        Test test = new Test();
        test.test(typo);
        System.out.println(typo.typpo());
    }
}

Test.java

package org.example;

public class Test {
    public void test(Typo typo) {
        System.out.println(typo.typpo());
    }
}

Typo.java

package org.example;

public interface Typo {
    String typpo();
}

First version result

shasum -a 256 first/build/classes/java/main/org/example/Main.class
11425f57f59d52ef0076638eb71d7478d6be2ab4a9e2b9ff5d1f89872022a7d4  first/build/classes/java/main/org/example/Main.class

Second version(https://github.com/cprayer/binary-compatibility-test-second-version)

Main.java

package org.example;

public class Main {
    public static void main(String[] args) {
        Typo typo = () -> "hello world";
        Test test = new Test();
        test.test(typo);
        System.out.println(typo.typpo());
    }
}

Test.java

package org.example;

public class Test {
    public void test(Typo typo) {
        System.out.println(typo.typo());
    }
}

Typo.java

package org.example;

public interface Typo {
    String typpo();

    default String typo() {
        return typpo();
    }
}

Second version result

shasum -a 256 second/build/classes/java/main/org/example/Main.class
11425f57f59d52ef0076638eb71d7478d6be2ab4a9e2b9ff5d1f89872022a7d4  second/build/classes/java/main/org/example/Main.class

I think it doesn't have binary compatibility issue.

@wilkinsona wilkinsona removed the for: merge-with-amendments Needs some changes when we merge label Mar 5, 2021
@wilkinsona
Copy link
Member

wilkinsona commented Mar 5, 2021

That's really helpful, @cprayer. Thank you. In addition to that check, I've built 2.4.4-SNAPSHOT locally and used Spring Cloud Context 3.0.1 with it. org.springframework.cloud.bootstrap.TextEncryptorConfigBootstrapper.intitialize(BootstrapRegistry) is called as expected via the initialize default method.

@wilkinsona wilkinsona self-assigned this Mar 5, 2021
@spencergibb
Copy link
Member

Thanks for checking on that @wilkinsona.

@wilkinsona
Copy link
Member

@cprayer Thanks very much for making your first contribution to Spring Boot.

@wilkinsona wilkinsona modified the milestones: 2.4.x, 2.4.4 Mar 5, 2021
izeye added a commit to izeye/spring-boot that referenced this pull request Mar 10, 2021
This was referenced Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants