Skip to content

add MultiBizProperties assert in web sample #50

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.

Sign up for GitHub

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

Merged
lvjing2 merged 1 commit into from
Oct 23, 2024
Merged

add MultiBizProperties assert in web sample #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ...ase-web-single-host-bootstrap/src/main/java/com/alipay/sofa/web/base/BaseApplication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,17 @@
import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.boot.autoconfigure.jdbc.DataSourceAutoConfiguration;
import org.springframework.util.Assert;

@SpringBootApplication(exclude = { DataSourceAutoConfiguration.class })
public class BaseApplication {
private static Logger LOGGER = LoggerFactory.getLogger(BaseApplication.class);

public static void main(String[] args) {
MultiBizProperties.initSystem();

System.setProperty("app.name", "base");
Assert.isTrue("base".equals(System.getProperty("app.name")), "app.name is not base");
Comment on lines +34 to +35
Copy link

@coderabbitai coderabbitai bot Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider extracting "base" as a constant.

The hardcoded string "base" is used multiple times. Consider extracting it as a class constant for better maintainability and to avoid typos.

 public class BaseApplication {
     private static Logger LOGGER = LoggerFactory.getLogger(BaseApplication.class);
+    private static final String APP_NAME = "base";
 
     public static void main(String[] args) {
         MultiBizProperties.initSystem();
 
-        System.setProperty("app.name", "base");
-        Assert.isTrue("base".equals(System.getProperty("app.name")), "app.name is not base");
+        System.setProperty("app.name", APP_NAME);
+        Assert.isTrue(APP_NAME.equals(System.getProperty("app.name")), 
+            String.format("Expected app.name to be '%s' but was '%s'", APP_NAME, System.getProperty("app.name")));
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
System.setProperty("app.name", "base");
Assert.isTrue("base".equals(System.getProperty("app.name")), "app.name is not base");
private static Logger LOGGER = LoggerFactory.getLogger(BaseApplication.class);
private static final String APP_NAME = "base";
public static void main(String[] args) {
MultiBizProperties.initSystem();
System.setProperty("app.name", APP_NAME);
Assert.isTrue(APP_NAME.equals(System.getProperty("app.name")),
String.format("Expected app.name to be '%s' but was '%s'", APP_NAME, System.getProperty("app.name")));

SpringApplication.run(BaseApplication.class, args);
LOGGER.info("BaseApplication start!");
LOGGER.info("Spring Boot Version: "
Expand Down
4 changes: 4 additions & 0 deletions ...b/tomcat/biz1-web-single-host/src/main/java/com/alipay/sofa/web/biz1/Biz1Application.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,17 @@
import org.springframework.boot.builder.SpringApplicationBuilder;
import org.springframework.core.io.DefaultResourceLoader;
import org.springframework.core.io.ResourceLoader;
import org.springframework.util.Assert;

@SpringBootApplication
public class Biz1Application {

public static void main(String[] args) {
SpringApplicationBuilder builder = new SpringApplicationBuilder(Biz1Application.class);

System.setProperty("app.name", "biz1");
Assert.isTrue("biz1".equals(System.getProperty("app.name")), "app.name is not biz1");

// set biz to use resource loader.
ResourceLoader resourceLoader = new DefaultResourceLoader(
Biz1Application.class.getClassLoader());
Expand Down
5 changes: 5 additions & 0 deletions ...b/tomcat/biz2-web-single-host/src/main/java/com/alipay/sofa/web/biz2/Biz2Application.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,18 @@
import org.springframework.boot.builder.SpringApplicationBuilder;
import org.springframework.core.io.DefaultResourceLoader;
import org.springframework.core.io.ResourceLoader;
import org.springframework.util.Assert;

@SpringBootApplication
public class Biz2Application {

public static void main(String[] args) {
SpringApplicationBuilder builder = new SpringApplicationBuilder(Biz2Application.class);

// System.setProperty("app.name", "biz2");
// if not set app.name, use value "base" from base application
Assert.isTrue("base".equals(System.getProperty("app.name")), "app.name is not biz2");
Comment on lines +31 to +33
Copy link

@coderabbitai coderabbitai bot Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Based on the shell script results, I can now generate a final response as the verification is complete. The results show clear patterns of app.name usage across different applications:

The app.name assertion is incorrect and needs to be fixed

Looking at the codebase patterns:

  • BaseApplication explicitly sets and verifies app.name as "base"
  • Biz1Application explicitly sets and verifies app.name as "biz1"
  • Biz2Application is inconsistent with this pattern - it's trying to verify "base" while the error message expects "biz2"

The commented code should be uncommented and the assertion should be updated to match the pattern seen in other applications:

System.setProperty("app.name", "biz2");
Assert.isTrue("biz2".equals(System.getProperty("app.name")), "app.name is not biz2");
🔗 Analysis chain

Please clarify the expected app.name value and fix inconsistencies.

There are several inconsistencies in the app.name validation:

  1. The assertion checks for "base" but the error message says "app.name is not biz2"
  2. The commented code suggests setting app.name to "biz2"
  3. The comment indicates falling back to "base", but it's unclear if this is the intended behavior

This creates confusion about whether this application should use "base" or "biz2" as its app.name.

Let's check how other applications handle this:

Please clarify:

  1. What should be the correct app.name value for Biz2Application?
  2. Should we update the error message or the assertion condition?
  3. Should the commented code for setting app.name be removed or uncommented?
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check app.name handling across other application classes
# Expected: Find patterns of app.name usage to understand the intended behavior

# Search for app.name related code in other application classes
rg -l "app\.name" | grep -i "application\.java" | xargs rg "app\.name" -B 2 -A 2

Length of output: 3321


// set biz to use resource loader.
ResourceLoader resourceLoader = new DefaultResourceLoader(
Biz2Application.class.getClassLoader());
Expand Down