4
\$\begingroup\$

We have a (Spring) REST API that I was tasked to write integration tests for. I wrote various ways to test our REST API and then just decided to make 1 integration test class that processes all JSON files that can be submitted in PUT requests against our REST API and also tests security and various GETs and DELETEs. This test class currently makes 586 HTTP (GET, PUT and DELETE) requests generally at or under a total of 30 seconds (about 20 HTTP requests/second). I decided to write my integration test without any 3rd party libs/frameworks after having frustration with Spring, Apache Http Components and other libs. At first I split up my test into several classes but then just decided to put it all into 1 class. I typically write small classes but wanted to experiment with 1 larger class to see what that looked/felt like. This code uses no 3rd party libs and only requires JSON files and a running REST API to process.

What are your thoughts about how I've set this up? Would you despise being assigned to modify this code if you were one of my coworkers? If you think this class should be "split" up, what advantages would be gained?

I developed this in IntelliJ IDEA which is currently reporting no warnings/errors on the class.

import java.io.*;
import java.net.InetAddress;
import java.net.Socket;
import java.net.UnknownHostException;
import java.nio.charset.StandardCharsets;
import java.nio.file.*;
import java.util.*;

// These tests expect the enable assertions flag ("-ea") to be set as a VM option
public final class RestApiIntTests {

    private static final String BASIC_AUTH_TOKEN = Base64.getEncoder().encodeToString("sa:sa".getBytes(StandardCharsets.UTF_8));

    private static final Map<String,String> STANDARD_HEADERS = new HashMap<String,String>() {{
        put("Authorization", "Basic " + BASIC_AUTH_TOKEN);
        put("Content-Type", "application/json");
        put("Accept", "application/json");
    }};

    private enum HttpVerb {
        DELETE, GET, PUT
    }

    public static void main(final String[] args) throws Exception {

        final long startTime = new Date().getTime();

        // Maps json filenames to corresponding REST endpoint
        final Map<String,String> jsonFilenameToEndpointMapper = new TreeMap<String,String>() {{
            put("alert","/alerts");
            put("chart","/charts");
            put("clientCustom","/clientCustom");
            put("connection","/connections");
            put("group","/groups");
            put("report","/reports");
            put("role","/roles");
            put("service","/services");
            put("user","/users");
        }};

        // ***********************************
        // Test GET requests at root endpoints
        // ***********************************
        for (final Map.Entry<String, String> entry : jsonFilenameToEndpointMapper.entrySet()) {
            final String curEndpoint = entry.getValue();
            System.out.println("get " + curEndpoint);
            final String getResp = restCall(HttpVerb.GET, "", STANDARD_HEADERS, curEndpoint);
            assert getResp.split("200 OK", -1).length-1 > 0 : getResp;
        }

        // **********
        // JSON TESTS
        // **********
        try (final DirectoryStream<Path> stream = Files.newDirectoryStream(Paths.get(""))) {
            final TreeSet<String> fileNames = new TreeSet<>();

            // First, gather file names for processing
            for (final Path file: stream) {
                final String fileName = file.getFileName().toString();

                // Specify whatever regex you want for matching test json files...
                if (fileName.matches(".*.json")) {
                    fileNames.add(fileName);
                }
            }

            // Now, process files in order
            for (final String fileName : fileNames) {
                final String curJsonFileName = fileName.substring(0, fileName.indexOf("."));

                final String curEndpoint = getCurrentEndpoint(curJsonFileName, jsonFilenameToEndpointMapper);
                final String curNameEndpoint = curEndpoint + "/restApiIntTest";  // "restApiIntTest" should be the "name" field value in the json files

                if (curJsonFileName.equals("clientCustom")) {

                    final String fieldToken = "\"applicationName\":";

                    // ********************
                    // Update Client Custom
                    // ********************
                    System.out.println("update " + curJsonFileName);
                    final String[] respLines = restCall(HttpVerb.GET, "", STANDARD_HEADERS, curEndpoint).split("\r\n");

                    final String clientCustomJson = extractJsonFromResponse(respLines, fieldToken);

                    final int startIndex = clientCustomJson.indexOf(fieldToken) + fieldToken.length();
                    final int endIndex = clientCustomJson.indexOf(",\"", startIndex);
                    final String origAppName = clientCustomJson.substring(startIndex, endIndex);

                    final String newAppName = "\"IR360-DEV-" + new Date().getTime() + "\"";

                    final String newClientCustomJson = clientCustomJson.replace(fieldToken + origAppName, fieldToken + newAppName);

                    final String putResp = restCall(HttpVerb.PUT, newClientCustomJson, STANDARD_HEADERS, curEndpoint);
                    assert putResp.toLowerCase().contains("success") : putResp;

                    final String[] newRespLines = restCall(HttpVerb.GET, "", STANDARD_HEADERS, curEndpoint).split("\r\n");
                    final String newClientCustomResp = extractJsonFromResponse(newRespLines, fieldToken);

                    assert newClientCustomResp.contains(newAppName) : newClientCustomResp;

                } else {

                    // ******
                    // Create
                    // ******
                    System.out.println("create " + curJsonFileName);
                    final InputStream inputStream = ClassLoader.getSystemClassLoader().getResourceAsStream(curJsonFileName + ".json");
                    final String curFileJson = new Scanner(inputStream, StandardCharsets.UTF_8.name()).useDelimiter("\\A").next();

                    // Make sure record doesn't exist before creating...
                    restCall(HttpVerb.DELETE, "", STANDARD_HEADERS, curNameEndpoint);

                    final String createResp = restCall(HttpVerb.PUT, curFileJson, STANDARD_HEADERS, curEndpoint);
                    assert createResp.toLowerCase().contains("success") : createResp;

                    // ***
                    // Get
                    // ***
                    System.out.println("get " + curJsonFileName);
                    final String getResp = restCall(HttpVerb.GET, "", STANDARD_HEADERS, curNameEndpoint);
                    assert getResp.split("\"guid\"", -1).length-1 > 0 : getResp;

                    // ******
                    // Update
                    // ******
                    System.out.println("update " + curJsonFileName);

                    // Expects original json fields/values setup like this...
                    // "<curUpdateField>":""
                    // "modifiedBy":""
                    // "modifiedDate":null
                    final String updatedDesc = "\"RestApiIntTests updated description...\"";
                    final String updateField = curJsonFileName.equals("user") ? "\"comment\":" : "\"description\":";
                    final String updatedJson = extractJsonFromResponse(getResp.split("\r\n"), "\"guid\":")
                                                .replace(updateField + "\"\"", updateField + updatedDesc)
                                                .replace("\"modifiedBy\":\"\"", "\"modifiedBy\":\"test\"")
                                                .replace("\"modifiedDate\":null", "\"modifiedDate\":\"" + new Date().getTime() + "\"");

                    final String updResp = restCall(HttpVerb.PUT, updatedJson, STANDARD_HEADERS, curEndpoint);
                    assert ( ! updResp.toLowerCase().contains("error")) : updResp;
                    final String updGetResp = restCall(HttpVerb.GET, "", STANDARD_HEADERS, curNameEndpoint);
                    assert updGetResp.contains(updatedDesc) : updGetResp;

                    // ******
                    // Delete
                    // ******
                    System.out.println("delete " + curJsonFileName);
                    final String delResp = restCall(HttpVerb.DELETE, "", STANDARD_HEADERS, curNameEndpoint);
                    assert delResp.toLowerCase().contains("success") : delResp;
                }
            }
        } catch (IOException | DirectoryIteratorException x) {
            // IOException can never be thrown by the iteration.
            // In this snippet, it can only be thrown by newDirectoryStream.
            System.err.println(x);
        }

        // **************
        // SECURITY TESTS
        // **************

        // ***************
        // Basic Auth Test
        // ***************
        System.out.println("basicAuthTest");
        final String basicAuthResp = restCall(HttpVerb.GET, "", new HashMap<String, String>() {{
            put("Authorization", "Basic " + BASIC_AUTH_TOKEN);
        }}, "/");
        assert basicAuthResp.toLowerCase().contains("hello") : basicAuthResp;

        // *********************
        // Remember Me Auth Test
        // *********************
        System.out.println("rememberMeAuthTest");

        // Do initial request with basic auth and get the remember-me cookie
        final String rememberMeNoExp = getRememberMeCookie(null);
        assert (rememberMeNoExp != null) && (rememberMeNoExp.length() > 0);

        // Now make a request with only the remember-me cookie present
        final String rememberMeResp = restCall(HttpVerb.GET, "", new HashMap<String, String>() {{
            put("remember-me", rememberMeNoExp);
        }}, "/");
        assert rememberMeResp.toLowerCase().contains("hello") : rememberMeResp;

        // ************************************
        // Remember Me Auth Cookie Expired Test
        // ************************************
        System.out.println("rememberMeAuth_CookieExpiredTest");

        // Do initial request with basic auth and get the remember-me cookie
        // (set the "tokenValiditySeconds" header to a low value to expire it right away)
        final String rememberMeExp = getRememberMeCookie(1);
        assert (rememberMeExp != null) && (rememberMeExp.length() > 0);

        // Now make a request with only the remember-me cookie present (should throw Exception as it will be expired when called)
        Thread.sleep(2000);
        final String rememberMeExpResp = restCall(HttpVerb.GET, "", new HashMap<String, String>() {{
            put("remember-me", rememberMeExp);
        }}, "/");
        assert rememberMeExpResp.toLowerCase().contains("unauthorized") : rememberMeExpResp;

        // **************
        // X509 Auth Test
        // **************
        System.out.println("x509AuthTest");

        /*
         * This is an X.509 certificate encoded as a Base64 String
         * If this String doesn't work, you can replace it using the following steps
         *  1. Generate a keypair using the JDK's "keytool" utility
         *     For example, the following creates a key with a common name of "sa" (the common name is extracted from the cert and used as the username to login to IR360),
         *     an alias of "x509" and a validity of 3650 days (about 10 years)
         *
         *          keytool -genkeypair -dname "cn=sa, ou=dev, o=avada, c=US" -alias x509 -validity 3650
         *
         *  2. Export the cert onto your machine (the following will create "x509test.cer" in your current directory)
         *
         *          keytool -exportcert -alias x509 -file x509test.cer
         *
         *  3. Base64 encode the contents of the exported cert and replace the String below with that value
         *     For example, open the file using Notepad++, select all and then choose Plugins -> MIME Tools -> Base64 Encode
         */
        final String x509Resp = restCall(HttpVerb.GET, "", new HashMap<String, String>() {{
            put("X.509", "MIICzTCCAougAwIBAgIEKdxA4zALBgcqhkjOOAQDBQAwODELMAkGA1UEBhMCVVMxDjAMBgNVBAoTBWF2YWRhMQwwCgYDVQQLEwNkZXYxCzAJBgNVBAMTAnNhMB4XDTE2MDUwNTIxMDQwMVoXDTI2MDUwMzIxMDQwMVowODELMAkGA1UEBhMCVVMxDjAMBgNVBAoTBWF2YWRhMQwwCgYDVQQLEwNkZXYxCzAJBgNVBAMTAnNhMIIBuDCCASwGByqGSM44BAEwggEfAoGBAP1/U4EddRIpUt9KnC7s5Of2EbdSPO9EAMMeP4C2USZpRV1AIlH7WT2NWPq/xfW6MPbLm1Vs14E7gB00b/JmYLdrmVClpJ+f6AR7ECLCT7up1/63xhv4O1fnxqimFQ8E+4P208UewwI1VBNaFpEy9nXzrith1yrv8iIDGZ3RSAHHAhUAl2BQjxUjC8yykrmCouuEC/BYHPUCgYEA9+GghdabPd7LvKtcNrhXuXmUr7v6OuqC+VdMCz0HgmdRWVeOutRZT+ZxBxCBgLRJFnEj6EwoFhO3zwkyjMim4TwWeotUfI0o4KOuHiuzpnWRbqN/C/ohNWLx+2J6ASQ7zKTxvqhRkImog9/hWuWfBpKLZl6Ae1UlZAFMO/7PSSoDgYUAAoGBALkISGRy+xusKkizEYMft0f3f3y+UONIWmSQ8ZqkxGT5WIr7msdyEdbxMBrL3ej1gZIjXDI6oLJfgH+M7lDMuKlsR4h96iXAQFIrZFWqjBvh/wFmgbsBzisTEE4NFsfXSAc6WJK6jnq3+6HRr4wH/lvN76oDPsFRBwZ48GUNi+iXoyEwHzAdBgNVHQ4EFgQUrg29qmlxC9T18Rmn/JUcD1RhxV4wCwYHKoZIzjgEAwUAAy8AMCwCFENOqc2FsDAc+uQz+/fafBVsf/RiAhQRAjEKobdD9eNbZCIiNgRYAbeDQQ==");
        }}, "/");
        assert x509Resp.toLowerCase().contains("hello") : x509Resp;

        // *********************
        // Output total run time
        // *********************
        final long totalSeconds = (new Date().getTime() - startTime) / 1000;
        System.out.println("Total run time seconds=" + totalSeconds);
    }

    private static String getCurrentEndpoint(final String curJsonFileName, final Map<String,String> jsonFilenameToEndpointMapper) {
        for (final Map.Entry<String, String> entry : jsonFilenameToEndpointMapper.entrySet()) {
            if (curJsonFileName.startsWith(entry.getKey())) {
                return entry.getValue();
            }
        }
        return "";
    }

    private static String extractJsonFromResponse(final String[] responseLines, final String token) {
        for (final String s : responseLines) {
            if (s.contains(token)) {
                return s.substring(0, s.lastIndexOf("}") + 1);
            }
        }
        return "";
    }

    private static String getRememberMeCookie(final Integer tokenValiditySeconds) throws Exception {
        final String uri = (tokenValiditySeconds == null) ? "/?remember-me=true" : "/?remember-me=true&tokenValiditySeconds=" + tokenValiditySeconds;

        final String[] responseLines = restCall(HttpVerb.GET, "", new HashMap<String, String>() {{ put("Authorization", "Basic " + BASIC_AUTH_TOKEN); }}, uri).split("\r\n");
        final String startToken = "remember-me=";
        for (final String line : responseLines) {
            if (line.contains(startToken)) {
                final int start = line.indexOf(startToken) + startToken.length();
                final int end = line.indexOf("; ", start);
                return line.substring(start, end);
            }
        }
        return null;
    }

    private static final InetAddress INET_ADDRESS = getInetAddress();
    private static InetAddress getInetAddress() {
        try {
            return InetAddress.getByName("localhost");
        } catch (UnknownHostException e) {
            System.err.println(e.getMessage());
            System.exit(1);
        }
        return null;
    }

    private static String restCall(final HttpVerb httpVerb, final String reqBody, final Map<String,String> headers, final String path) {
        try (final Socket socket = new Socket(INET_ADDRESS, 8080);
             final BufferedWriter bw = new BufferedWriter(new OutputStreamWriter(socket.getOutputStream(), StandardCharsets.UTF_8));
             final BufferedReader br = new BufferedReader(new InputStreamReader(socket.getInputStream())))
        {
            socket.setSoTimeout(30000); // milliseconds

            // Make HTTP request
            // Not sure why, but content somehow has \u0000 in it...need to strip those out...
            final String content = reqBody.replace("\u0000", "");
            bw.write(httpVerb.toString() + " /IR360/rest" + path + " HTTP/1.0\r\n");
            for (Map.Entry<String,String> entry : headers.entrySet()) {
                bw.write(entry.getKey() + ": " + entry.getValue() + "\r\n");
            }
            if (content.length() > 0) {
                bw.write("Content-Length: " + content.length() + "\r\n");
            }
            bw.write("\r\n");
            if (content.length() > 0) {
                bw.write(content);
            }
            bw.flush();

            // Process HTTP response
            final StringBuilder sb = new StringBuilder();
            int i;
            while( (i = br.read()) != -1){
                sb.append((char)i);
            }

            return sb.toString();

        } catch (Exception e) {
            System.err.println("Problem communicating with REST API : " + e);
            System.exit(1);
        }

        return "--";
    }
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

If you think this class should be "split" up, what advantages would be gained?

People would be able to read this code in an acceptable time and maybe even modify it, if it is required.

You are asking about splitting code into multiple classes, but I think that you should take a step back and focus on splitting into multiple methods. Your code is almost one big method which does everything. It looks like you feel that it needs to be split – you use comments to mark sections.

Why do I have to read your whole code if I only want to understand what it does? People who work with existing code are mostly interested to understand what code does, not how it does. You need to abstract things. Classes, methods are tools that help to achieve this.

\$\endgroup\$
3
  • \$\begingroup\$ Not a bad point...What is unreadable about the code? \$\endgroup\$ Nov 21, 2016 at 15:42
  • \$\begingroup\$ Good points on the comments being marked as sections. Normally, as I said in my post, I write very small classes/methods but I've been finding it hard to navigate my object trees because of so many small classes/methods that I've been experimenting with large classes/methods. \$\endgroup\$ Nov 21, 2016 at 20:04
  • 1
    \$\begingroup\$ I understand that is only an experiment. If your problem is navigating, try to learn features of your IDE. Tap shift twice and type what you are looking for \$\endgroup\$
    – Karol
    Nov 21, 2016 at 20:44
1
\$\begingroup\$

Right, so I'm going to assume this is formatted according to your style guide.

Next, use a unit test library instead of assert, in fact assert should be used for asserting things in regular programs that "can't possibly happen", that's why it can even be disabled for production.

There's probably an option to run this in the same JVM as the actual endpoints to prevent actually going onto the wire - maybe look into that if the time spent on HTTP calls is too big, or consider running tests concurrently too.

Are all the finals mandatory? Otherwise I'd drop that, it doesn't help readability very much.

The initialisation for x509Resp is, uh, horrible, just put the certificate into a separate resource file.

Returning a viable value in an error case is bad, for getCurrentEndpoint and extractJsonFromResponse consider throwing an exception if the value wasn't found, or return null or something, but make sure that the empty string doesn't end up being used - that would debugging also a bit easier.

Catching an exception only to call System.exit seems pointless, just let it propagate and the exception will be printed anyway.

Why on earth is the HTTP request being created manually? Take a look at one of the libraries for that and use them. E.g. Jersey is nice for a variety of web services, but if that's too much of a change perhaps just Apache HttpClient is fine too. All the string fiddling should also go away if JSON is parsed properly and libraries are used instead.

Why is there a Thread.sleep in there? The comment doesn't explain it, so I'd think it's unnecessary.

So yeah, not all bad, just use libraries and move logic into methods at least, a function (main) this long is somewhat unreadable and e.g. the big if/else blocks are really candidates to be moved into separate methods too.

\$\endgroup\$
5
  • \$\begingroup\$ I appreciate your review. If I can accomplish the same thing without 3rd party libs, why rope them in? Concerning the finals - keeping variables final keeps state from being mucked with (except in collections) and immutability is much easier to deal with than mutable objects. \$\endgroup\$ Nov 21, 2016 at 20:02
  • 1
    \$\begingroup\$ Why libraries? Because not using them forces you to reimplement things that have been solved, which is a source of possible mistakes and missing handling of edge cases, it takes time and adds to the list of things that will need to be understood and maintained. Regarding final - it's not const, the objects aren't magically immutable afterwards, just the variable assignments can't be changed. \$\endgroup\$
    – ferada
    Nov 21, 2016 at 20:06
  • 1
    \$\begingroup\$ BTW, as I commented above, I normally write small classes/methods but I've been finding my code is sometimes hard to navigate in the object tree because of having so many small classes/methods...I decided to experiment with 1 large class/method to see what that "feels" like...it's obviously not very readable...thanks again. \$\endgroup\$ Nov 21, 2016 at 20:08
  • \$\begingroup\$ Find a middle ground, if the main is a bit more concise and uses methods in the same class it's fine IMO. \$\endgroup\$
    – ferada
    Nov 21, 2016 at 20:10
  • 1
    \$\begingroup\$ Once again, I really appreciate your review. Your criticisms are warranted. I work on a team where other coders tend to program with large classes/methods and I've been criticized for having/advocating for small classes/methods. I wonder if there's a happy "medium" of not having too small/large a class/method... \$\endgroup\$ Nov 21, 2016 at 20:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.