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

Need more explicit exceptions so its easier to test in application code #488

Open
wdroste opened this issue Nov 7, 2022 · 4 comments
Open
Labels
type/enhancement Type: make the code neat or more efficient

Comments

@wdroste
Copy link
Contributor

wdroste commented Nov 7, 2022

For instance

new RuntimeException(
            "Switch space `"
            + config.getSpaceName()
            + "' failed: "
            + resultSet.getErrorMessage());

If this was a NoSpaceFoundException it would be easier to test. Frequently we'd like know if an exception is one we can retry so having ones that implement TransitentException or some ClientException is helpful.

For instance this NoSpaceFoundException is not retryable.

@Sophie-Xie Sophie-Xie added the type/enhancement Type: make the code neat or more efficient label Nov 30, 2022
@Nicole00
Copy link
Contributor

The client exposed the ResultSet's errorCode to users and expect users handle the business code according to errorCode. Can you check the errorCode type to decide whether you can try the statement?

@wdroste
Copy link
Contributor Author

wdroste commented Jan 2, 2023

@Nicole00 I'm confused, the error codes are present in the code however, the exception thrown from library does not contain the code or a clarifying exception.

getSessionWrapper()

        // create new session
        try {
            Session session =
                    pool.getSession(
                            config.getUserName(), config.getPassword(), config.isReconnect());
            ResultSet resultSet = session.execute("USE " + config.getSpaceName());
            if (!resultSet.isSucceeded()) {
                throw new RuntimeException(
                        "Switch space `"
                                + config.getSpaceName()
                                + "' failed: "
                                + resultSet.getErrorMessage());
            }
            SessionWrapper sessionWrapper = new SessionWrapper(session);
            sessionList.add(sessionWrapper);
            return sessionWrapper;
        } catch (AuthFailedException | NotValidConnectionException | IOErrorException e) {
            throw new RuntimeException("Get session failed: " + e.getMessage());
        }

This is from SessionManager.java basically it just throws RuntimeException wtihout an error code so the user would need to parse the error message.

Basically how does this code tell the client its a AuthFailedException or a Switch space, as in either case it throws RuntimeException.

@wdroste
Copy link
Contributor Author

wdroste commented Jan 2, 2023

This is a common pattern in the library code it throws a lot of plain RuntimeException with out passing the error code or a clarifying exception. Basically taking AuthFailedException and have it inherit from RuntimeException would help. Or better yet a basse ClientException with the error code and then extend it from there for something like AuthFailedFailed or CertificateException etc.

@Nicole00
Copy link
Contributor

Nicole00 commented May 8, 2023

I agree with the view on runtime exception, it should indeed throw the specific exception. What I mean above is about the errorCode returned by server, it is contained in ResultSet.

Perhaps the SessionWrapper will be Deprecated later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Type: make the code neat or more efficient
Projects
None yet
Development

No branches or pull requests

3 participants