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.

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

Jump to bottom

fix: set default 0 when get method timeout config #1422

Merged
merged 2 commits into from
Jul 29, 2024
Merged

fix: set default 0 when get method timeout config #1422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e56de78
fix: default 0 when get method timeout config
Jun 13, 2024
2ffde4c
format
Jul 2, 2024
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
2 changes: 1 addition & 1 deletion core/api/src/main/java/com/alipay/sofa/rpc/config/MethodConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ public MethodConfig setParameters(Map<String, String> parameters) {
* @return the timeout
*/
public Integer getTimeout() {
return timeout;
return timeout == null ? 0 : timeout;
}

/**
Expand Down
16 changes: 16 additions & 0 deletions core/api/src/test/java/com/alipay/sofa/rpc/std/config/AbstractInterfaceConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.alipay.sofa.rpc.config.AbstractInterfaceConfig;
import com.alipay.sofa.rpc.config.ApplicationConfig;
import com.alipay.sofa.rpc.config.MethodConfig;
import com.alipay.sofa.rpc.config.ProviderConfig;
import com.alipay.sofa.rpc.config.RegistryConfig;
import com.alipay.sofa.rpc.core.exception.SofaRpcRuntimeException;
import com.alipay.sofa.rpc.listener.ConfigListener;
Expand Down Expand Up @@ -50,6 +51,21 @@
* @version : AbstractInterfaceConfigTest.java, v 0.1 2022年01月25日 4:46 下午 zhaowang
*/
public class AbstractInterfaceConfigTest {
@Test
public void testMethodTimeout() {
MethodConfig config = new MethodConfig();
config.setTimeout(null);

ProviderConfig p = new ProviderConfig();
p.setMethods(new HashMap<>());
p.getMethods().put("test", config);

try {
Assert.assertFalse(p.hasTimeout());
} catch (Exception e) {
Assert.fail("exception should not appears: " + e.getMessage());
}
}
Comment on lines +54 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding more test scenarios for robust testing.

The test method testMethodTimeout effectively checks the behavior when timeout is null. Consider adding more scenarios to ensure robust testing, such as verifying the actual timeout value used by the system when null is set.

Would you like me to help create additional test cases for this scenario?


@Test
public void testDefaultValue() {
Expand Down
11 changes: 11 additions & 0 deletions ...registry-sofa/src/test/java/com/alipay/sofa/rpc/registry/sofa/SofaRegistryHelperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,17 @@ public void convertProviderToUrls() throws Exception {
Assert.assertEquals(serverConfig3.getPort(), providerInfo3.getPort());
Assert.assertEquals(providerConfig.getAppName(), providerInfo3.getAttr(ProviderInfoAttrs.ATTR_APP_NAME));
Assert.assertEquals(providerConfig.getTimeout(), providerInfo3.getDynamicAttr(ProviderInfoAttrs.ATTR_TIMEOUT));

ProviderConfig<?> providerConfig2 = new ProviderConfig<>();
providerConfig2.setMethods(new HashMap<>());
providerConfig2.getMethods().put("test", new MethodConfig().setTimeout(null));
Assert.assertNotNull(providerConfig2.getTimeout());
String s4 = SofaRegistryHelper.convertProviderToUrls(providerConfig2, serverConfig);
Assert.assertNotNull(s3);
ProviderInfo providerInfo4 = SofaRegistryHelper.parseProviderInfo(s4);
Assert.assertEquals(0, providerInfo4.getDynamicAttr(".test.timeout"));
Assert.assertFalse(providerConfig2.hasTimeout());

Comment on lines +153 to +163
Copy link
Contributor

Choose a reason for hiding this comment

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

The modifications in the convertProviderToUrls method correctly handle the scenario where a method's timeout is null, ensuring a default value of 0 is used. This aligns with the PR's objectives to prevent null timeout values from being published. Consider verifying this behavior in integration tests to ensure it behaves as expected across different configurations.

Would you like assistance in setting up integration tests for this scenario?

}

@Test
Expand Down
Loading