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

fix:User_Logout_NPE #153

Merged
merged 7 commits into from
Sep 21, 2024
Merged

Conversation

cmgyqjj
Copy link
Contributor

@cmgyqjj cmgyqjj commented Sep 20, 2024

For bug: NullPointerException

add an online check in the logout process to prevent a NullPointerException (NPE)

@crossoverJie crossoverJie linked an issue Sep 21, 2024 that may be closed by this pull request
@@ -131,10 +131,12 @@ public BaseResponse<NULLBody> p2pRoute(@RequestBody P2PReqVO p2pRequest) throws
public BaseResponse<NULLBody> offLine(@RequestBody ChatReqVO groupReqVO) {
BaseResponse<NULLBody> res = new BaseResponse();

CIMUserInfo cimUserInfo = userInfoCacheService.loadUserInfoByUserId(groupReqVO.getUserId());
if(userInfoCacheService.CheckUserLoginStatus(groupReqVO.getUserId())){
Copy link
Owner

Choose a reason for hiding this comment

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

Why not directly modify the return value of loadUserInfoByUserId to Optional<CIMUserInfo>, and then call the offLine function in ifPresent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, It is a good idea that it will be midified right now.

@crossoverJie
Copy link
Owner

Additionally, for this scenario, we should add a new test case in ClientTest, like this:

    @Test
    public void testIncorrectUser() throws Exception {
        super.starSingleServer();
        super.startRoute();
        String routeUrl = "http://localhost:8083";
        String cj = "xx";
        long id = 100L;
        var auth1 = ClientConfigurationData.Auth.builder()
                .userId(id)
                .userName(cj)
                .build();

        Client client1 = Client.builder()
                .auth(auth1)
                .routeUrl(routeUrl)
                .build();
        TimeUnit.SECONDS.sleep(3);

        Assertions.assertDoesNotThrow(client1::close);

        super.stopSingle();
    }

@crossoverJie crossoverJie added bug Something isn't working enhancement New feature or request labels Sep 21, 2024
@crossoverJie crossoverJie added this to the 
v2.0.0 milestone Sep 21, 2024
@cmgyqjj
Copy link
Contributor Author

cmgyqjj commented Sep 21, 2024

Additionally, for this scenario, we should add a new test case in ClientTest, like this:

    @Test
    public void testIncorrectUser() throws Exception {
        super.starSingleServer();
        super.startRoute();
        String routeUrl = "http://localhost:8083";
        String cj = "xx";
        long id = 100L;
        var auth1 = ClientConfigurationData.Auth.builder()
                .userId(id)
                .userName(cj)
                .build();

        Client client1 = Client.builder()
                .auth(auth1)
                .routeUrl(routeUrl)
                .build();
        TimeUnit.SECONDS.sleep(3);

        Assertions.assertDoesNotThrow(client1::close);

        super.stopSingle();
    }

Okay, this test case will be added in this pull request.


String url = "http://" + cimServerResVO.getIp() + ":" + cimServerResVO.getHttpPort();
ServerApi serverApi = RpcProxyManager.create(ServerApi.class, okHttpClient);
SendMsgReqVO vo = new SendMsgReqVO(cimUserInfo.get().getUserName() + ":" + groupReqVO.getMsg(), groupReqVO.getUserId());
Copy link
Owner

Choose a reason for hiding this comment

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

It is not recommended to directly use the java.util.Optional#get method, as it defeats the purpose of using Optional. It is suggested that similar modifications be made in all relevant places.

        cimUserInfo.ifPresent(userInfo -> {
            String url = "http://" + cimServerResVO.getIp() + ":" + cimServerResVO.getHttpPort();
            SendMsgReqVO vo = new SendMsgReqVO(userInfo.getUserName() + ":" + groupReqVO.getMsg(), groupReqVO.getUserId());
            serverApi.sendMsg(vo, url);
        });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previously incorrect usage of the non-standard java.util.Optional#get method has been revised upon our re-examination.

set.add(cimUserInfo) ;
Optional<CIMUserInfo> cimUserInfo = loadUserInfoByUserId(Long.valueOf(member)) ;

cimUserInfo.ifPresentOrElse(set::add, () -> {});
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
cimUserInfo.ifPresentOrElse(set::add, () -> {});
cimUserInfo.ifPresent(set::add);

Copy link
Owner

@crossoverJie crossoverJie left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution

@crossoverJie crossoverJie merged commit 5bba820 into crossoverJie:master Sep 21, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: NullPointerException
2 participants