1
\$\begingroup\$

I have created this repository that implements this OAuth2 flow. I wanted to use Java and create a minimal implementation of this flow with these goals in mind:

  1. Develop unit and integration tests in a straightforward way
  2. No heavy framework usage, mostly "explicit" code
  3. Have some kind of pattern for domain classes, and a straightforward way to validate request data

I would like to get some feedback on these points specifically:

  1. No usage of any ORM framework, writing custom code to build SQL queries, for example:

    The last 4 methods of class Installation

    @JsonIgnore
    public String getInsertQuery() {
        return String.format("INSERT INTO %s %s VALUES %s", TABLE_NAME, getColumnNames(), getParameterPart());
    }
    
    
    @JsonIgnore
    public String getParameterPart() {
        return this.toJsonObject().stream()
            .filter(it -> it.getValue() != null)
            .map(it -> "?")
            .collect(Collectors.joining(", ", "(", ")"));
    }
    
    
    @JsonIgnore
    public String getColumnNames() {
        return this.toJsonObject().stream()
            .filter(it -> it.getValue() != null)
            .map(it -> it.getKey())
            .collect(Collectors.joining(", ", "(", ")"));
    }
    
    
    @JsonIgnore
    public JsonArray getInsertQueryParams() {
        return new JsonArray(
            this.toJsonObject().stream()
                .filter(it -> it.getValue() != null)
                .map(it -> it.getValue())
                .collect(Collectors.toList()));
    }
    
  2. Mixing Futures with callbacks, especially in this one most important and problematic method: AppInstallationVerticle::handleInstallationRequest()

    private Handler<Message<JsonObject>> handleInstallationRequest(String clientId, String clientSecret) {
        return message -> {
            InstallationRequest event = Model.fromJsonObject(message.body(), InstallationRequest.class);
    
    
            if (!event.hasValidSignature(clientSecret)) {
                String errorMsg = "Received installation request with invalid signature: " + event.toString();
                LOG.error(errorMsg);
                message.fail(400, errorMsg);
            } else {
    
    
                LOG.info("received installation event " + event.toString());
    
    
                OAuth2ClientOptions credentials = new OAuth2ClientOptions()
                    .setClientID(clientId).setClientSecret(clientSecret)
                    .setSite(event.apiUrl).setTokenPath(event.tokenPath);
    
    
                requestOAuth2Token(credentials, event.code, event.returnUrl).setHandler(tokenResponse -> {
    
    
                    if (tokenResponse.failed()) {
                        String errorMsg = String.format("could not get token for event %s because of %s",
                            event.toString(), tokenResponse.cause());
                        LOG.error(errorMsg);
                        message.fail(500, errorMsg);
                    } else {
                        AccessToken token = tokenResponse.result();
                        String accessToken = token.principal().getString("access_token");
                        LOG.info(String.format("obtained access token %s for API URL %s", accessToken, event.apiUrl));
                        getShopInfo(accessToken, event.apiUrl).setHandler(shopInfo -> {
                            if (shopInfo.failed()) {
                                String errorMsg = String.format("could not get shop info for event %s because of %s",
                                    event.toString(), shopInfo.cause());
                                LOG.error(errorMsg);
                                message.fail(500, errorMsg);
                            } else {
                                Future<JsonObject> installationCompleted = createInstallation(accessToken,
                                    shopInfo.result(), event);
                                installationCompleted.setHandler(installationResult -> {
                                    if (installationResult.failed()) {
                                        String errorMsg = String.format(
                                            "could not create installation for event %s because of %s",
                                            event.toString(), installationResult.cause());
                                        LOG.error(errorMsg);
                                        message.fail(500, errorMsg);
                                    } else {
                                        message.reply(installationResult.result());
                                    }
                                });
                            }
                        });
                    }
                });
            }
        };
    };
    
  3. Callback hell also in tests: e.g. AppInstallationVerticleTest::_testAppInstallationOauthDance()

    @Test
    public void testAppInstallationOauthDance(TestContext context) {
        Async async = context.async();
    
    
        // GIVEN
    
    
        CompositeFuture.all(apiMockStarted, databasePrepared).setHandler(started -> {
            if (started.failed()) {
                started.cause().printStackTrace();
                context.fail();
                async.complete();
            }
    
    
            vertx.deployVerticle(AppInstallationVerticle.class.getName(), deploymentOpts, deployed -> {
                if (deployed.failed()) {
                    deployed.cause().printStackTrace();
                    context.fail();
                    async.complete();
                }
    
    
                // WHEN
    
    
                vertx.eventBus().<JsonObject>send(
                    AppInstallationVerticle.EVENT_BUS_ADDRESS,
                    installationEvent.toJsonObject(),
                    response -> {
    
    
                        // THEN
                        context.assertTrue(response.succeeded(),
                            response.cause() != null ? response.cause().getMessage() : null);
                        context.assertEquals(null, response.result().body());
    
    
                        dbClient.getConnection(connected -> {
                            if (connected.failed()) {
                                connected.cause().printStackTrace();
                                context.fail();
                                async.complete();
                            }
                            connected.result().query(
                                String.format("SELECT access_token FROM installations WHERE api_url = '%s'",
                                    installationEvent.apiUrl),
                                result -> {
                                    if (result.failed()) {
                                        result.cause().printStackTrace();
                                        context.fail();
                                    }
                                    context.assertEquals(tokenResponse.getString("access_token"),
                                        result.result().getResults().get(0).getValue(0));
                                    context.assertEquals(1, result.result().getNumRows());
                                    async.complete();
                                });
                        });
                    });
            });
        });
        async.awaitSuccess(2000);
    }
    
\$\endgroup\$
  • \$\begingroup\$ The code formatting is unfortunately not always working for big blocks... Can we still move this to discussion? \$\endgroup\$ – Thomas Hirsch Nov 30 '17 at 9:38
  • \$\begingroup\$ @ThomasHirsch That happens because the automatic formatter tool doesn not like code in between lists. You could negate this by adding a line in between with --- \$\endgroup\$ – Ludisposed Nov 30 '17 at 12:41
  • \$\begingroup\$ Allright, thank you very much for correcting it! \$\endgroup\$ – Thomas Hirsch Nov 30 '17 at 15:08
  • \$\begingroup\$ @Ludisposed Is there an option to rename question in order to gain some attention? I'd change the title to "Java Microservice Development" or something... \$\endgroup\$ – Thomas Hirsch Feb 6 '18 at 10:33
  • 1
    \$\begingroup\$ @Ludisposed: Code in between lists is okay, but it requires more indentation (8 spaces instead of 4). I've just fixed that here. \$\endgroup\$ – Jamal Feb 11 '18 at 21:02
2
\$\begingroup\$
  1. No ORM - You don't want to use any ORM framework and that is fine. However you have to prevent SQL-injection to say the least. If your program takes client parameters as-is and/or you don't use PreparedStatements, then the application is vulnerable. I was in a similar situation: using Vertx and dislike ORMs.

    That is why I've chosen jOOQ for database communication. jOOQ's principle is 'schema first' and 'embrace SQL', so you take your (existing) database-schema, run the jOOQ code-generator and it will create DAOs, POJOs and the whole schema as Java Objects, so you can perform typesafe SQL. When I used it, I was missing some convenience features to execute jOOQ queries on a Vertx thread or transforming POJOs into JsonObjects. That is where I started writing vertx-jooq, which does all this. Maybe you want to give it a try.

  2. Mixing future with callbacks - First thing to mention: a vertx Future<T> extends Handler<AsyncResult<T>>. So all code that takes a Handler<AsyncResult<T>> as an argument (e.g. someMethod(Handler<AsyncResult<T>> handler)) can be used as follows:

    Future<T> future = Future.future();
    someMethod(future);
    

    Next step is to use composition or mapping instead of the setHandler-method to act on the Futures result.

    Instead of writing:

    requestOAuth2Token(credentials, event.code, event.returnUrl).setHandler(h->
    {
        if(h.succeeded()) 
            /* do something */ 
        else 
            /* handle error*/
    });
    

    you would write:

    requestOAuth2Token(credentials, event.code, event.returnUrl).compose(token -> 
    {
        String accessToken = token.principal().getString("access_token");
        LOG.info(String.format("obtained access token %s for API URL %s", accessToken, event.apiUrl));
        return getShopInfo(accessToken, event.apiUrl).compose(shopInfo ->
            createInstallation(accessToken,shopInfo, event);
        );
    });
    

    and only in the final step you can invoke setHandler to act on the result to send a response on success / failure. I also suggest to check the Future-API.

  3. See 2.

\$\endgroup\$
  • \$\begingroup\$ (Welcome to CR!) kudos for pointing out the necessity to address security concerns when avoiding tools. \$\endgroup\$ – greybeard Feb 8 '18 at 10:12
  • \$\begingroup\$ Thanks for your reply, much appreciated! Regarding SQL injection, I am generally aware of the problem, and I thought I alleviated that by using the parameterised queries API, i.e. passing a string (?, ?, ?), and the parameters separately. \$\endgroup\$ – Thomas Hirsch Feb 12 '18 at 16:01
  • \$\begingroup\$ @infinikli Just to clarify this, so in order to get the else branch in your example above, I'd need to call otherwise() on the first future? I think I tried that combination but somehow could not make it pass the tests... \$\endgroup\$ – Thomas Hirsch Feb 12 '18 at 23:27
  • \$\begingroup\$ Correct, either otherwise() or recover(). The former in case your function returns just a value (or null), the latter if your function returns a Future. \$\endgroup\$ – infinikli Feb 13 '18 at 9:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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