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

Event endpoint to handle notifications was implemented #296

Merged
merged 6 commits into from
Mar 7, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package org.prebid.server.analytics.model;

import lombok.Value;

@Value
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use static factory of method for consistency

public class NotificationEvent {
String type;

String bidId;

String bidder;
}
148 changes: 148 additions & 0 deletions src/main/java/org/prebid/server/handler/NotificationEventHandler.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
package org.prebid.server.handler;

import io.netty.handler.codec.http.HttpResponseStatus;
import io.vertx.core.Handler;
import io.vertx.core.buffer.Buffer;
import io.vertx.core.http.HttpHeaders;
import io.vertx.core.json.DecodeException;
import io.vertx.core.json.Json;
import io.vertx.ext.web.RoutingContext;
import org.apache.commons.lang3.ObjectUtils;
import org.apache.commons.lang3.StringUtils;
import org.prebid.server.analytics.AnalyticsReporter;
import org.prebid.server.analytics.model.HttpContext;
import org.prebid.server.analytics.model.NotificationEvent;
import org.prebid.server.exception.InvalidRequestException;
import org.prebid.server.proto.request.EventNotificationRequest;
import org.prebid.server.util.ResourceUtil;

import java.io.IOException;
import java.util.Map;
import java.util.Objects;

/**
* Accepts notifications from browsers and mobile application for further processing by {@link AnalyticsReporter}
* and responding with tracking pixel when requested.
*/
public class NotificationEventHandler implements Handler<RoutingContext> {

private static final String TRACKING_PIXEL_PNG = "static/tracking-pixel.png";
private static final String TRACKING_PIXEL_JPG = "static/tracking-pixel.jpg";
private static final String VIEW_TYPE = "view";
private static final String WIN_TYPE = "win";
private static final String FORMAT_PARAMETER = "format";
private static final String JPG = "jpg";
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's have these renamed to JPG_FORMAT and PNG_FORMAT correspondingly

private static final String PNG = "png";
private static final String JPG_HEADER = "image/jpeg";
Copy link
Contributor

Choose a reason for hiding this comment

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

These are rather JPG_CONTENT_TYPE and PNG_CONTENT_TYPE

private static final String PNG_HEADER = "image/png";

private final AnalyticsReporter analyticsReporter;
private final byte[] trackingPixelPng;
private final byte[] trackingPixelJpg;

private NotificationEventHandler(AnalyticsReporter analyticsReporter, byte[] trackingPixelPng,
byte[] trackingPixelJpg) {
this.analyticsReporter = analyticsReporter;
this.trackingPixelJpg = trackingPixelJpg;
this.trackingPixelPng = trackingPixelPng;
}

public static NotificationEventHandler create(AnalyticsReporter analyticsReporter) {
return new NotificationEventHandler(Objects.requireNonNull(analyticsReporter),
readTrackingPixel(TRACKING_PIXEL_PNG), readTrackingPixel(TRACKING_PIXEL_JPG));
}

private static byte[] readTrackingPixel(String path) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this method next to handle to have private methods after public ones

try {
return ResourceUtil.readByteArrayFromClassPath(path);
} catch (IOException e) {
throw new IllegalArgumentException(String.format("Failed to load pixel image at %s", path), e);
}
}

@Override
public void handle(RoutingContext context) {
final EventNotificationRequest eventNotificationRequest;
try {
eventNotificationRequest = makeEventRequest(context.getBody());
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that's correct - it is a GET request and all parameters should be read from query params.

Fixed tech spec to explicitly say it is a GET request

} catch (InvalidRequestException ex) {
respondWithBadStatus(context, ex.getMessage());
return;
}

final NotificationEvent notificationEvent = new NotificationEvent(eventNotificationRequest.getType(),
eventNotificationRequest.getBidId(), eventNotificationRequest.getBidder());

analyticsReporter.processEvent(notificationEvent);

final Map<String, String> queryParams = HttpContext.from(context).getQueryParams();
Copy link
Contributor

Choose a reason for hiding this comment

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

There is not need to create HttpContext for the sake of just reading a query parameter - it could be read from context directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Has this been addressed somehow?

final String format = queryParams.get(FORMAT_PARAMETER);

try {
validateEventRequestQueryParams(format);
} catch (InvalidRequestException ex) {
respondWithBadStatus(context, ex.getMessage());
return;
}

respondWithOkStatus(context, format);
}

private EventNotificationRequest makeEventRequest(Buffer body) {
if (body == null) {
throw new InvalidRequestException(
"Request body was empty. Expected request with body has next fields: type, bidid and bidder.");
}

final EventNotificationRequest eventNotificationRequest;
try {
eventNotificationRequest = Json.decodeValue(body, EventNotificationRequest.class);
} catch (DecodeException e) {
throw new InvalidRequestException(
"Request body couldn't be parsed. Expected request body has next fields: type, bidid and bidder.");
}

validateEventRequestBody(eventNotificationRequest);
return eventNotificationRequest;
}

private void validateEventRequestBody(EventNotificationRequest eventNotificationRequest) {
final String type = eventNotificationRequest.getType();
if (ObjectUtils.notEqual(type, VIEW_TYPE) && ObjectUtils.notEqual(type, WIN_TYPE)) {
throw new InvalidRequestException(String.format(
"Type is required parameter. Possible values are win and view, but was %s", type));
}

final String bidId = eventNotificationRequest.getBidId();
if (StringUtils.isBlank(bidId)) {
throw new InvalidRequestException("bidid is required and can't be empty.");
}

final String bidder = eventNotificationRequest.getBidder();
if (StringUtils.isBlank(bidder)) {
throw new InvalidRequestException("bidder is required and can't be empty.");
}

}

private void validateEventRequestQueryParams(String format) {
if (format != null && ObjectUtils.notEqual(format, JPG) && ObjectUtils.notEqual(format, PNG)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Starting from here and onwards: this kind of conditions is not scalable enough and may bite you when third or fourth format will be added, especially this one and alike: format.equals(JPG) ? JPG_HEADER : PNG_HEADER.
I suggest creating a map holding all the necessary information about supported format:

final Map<String, TrackingPixel> trackingPixels;
...
private static class TrackingPixel {
    String contentType;
    byte[] content
}

It will be easy to use it then for validation and response construction and it will be much safer when it will come to adding another format/

throw new InvalidRequestException("format when defined should has value of png or jpg.");
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to pull format parameter values from trackingPixels map

}
}

private void respondWithBadStatus(RoutingContext context, String message) {
context.response().setStatusCode(HttpResponseStatus.BAD_REQUEST.code())
.end(String.format("%s%s", "Request is invalid: ", message));
Copy link
Contributor

Choose a reason for hiding this comment

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

String.format("Request is invalid: %s", message) looks more readable, isn't it?

}

private void respondWithOkStatus(RoutingContext context, String format) {
if (format != null) {
context.response()
.putHeader(HttpHeaders.CONTENT_TYPE, format.equals(JPG) ? JPG_HEADER : PNG_HEADER)
.end(format.equals(JPG) ? Buffer.buffer(trackingPixelJpg) : Buffer.buffer(trackingPixelPng));
} else {
context.response().end();
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package org.prebid.server.proto.request;

import com.fasterxml.jackson.annotation.JsonProperty;
import lombok.Builder;
import lombok.Value;

@Builder
@Value
public class EventNotificationRequest {

String type;

@JsonProperty("bidid")
String bidId;

String bidder;

String format;
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.prebid.server.handler.CurrencyRatesHandler;
import org.prebid.server.handler.ExceptionHandler;
import org.prebid.server.handler.NoCacheHandler;
import org.prebid.server.handler.NotificationEventHandler;
import org.prebid.server.handler.OptoutHandler;
import org.prebid.server.handler.SettingsCacheNotificationHandler;
import org.prebid.server.handler.SetuidHandler;
Expand Down Expand Up @@ -146,6 +147,7 @@ Router router(CookieHandler cookieHandler,
BidderParamHandler bidderParamHandler,
BiddersHandler biddersHandler,
BidderDetailsHandler bidderDetailsHandler,
NotificationEventHandler notificationEventHandler,
StaticHandler staticHandler) {

final Router router = Router.router(vertx);
Expand All @@ -164,6 +166,7 @@ Router router(CookieHandler cookieHandler,
router.get("/bidders/params").handler(bidderParamHandler);
router.get("/info/bidders").handler(biddersHandler);
router.get("/info/bidders/:bidderName").handler(bidderDetailsHandler);
router.post("/event").handler(notificationEventHandler);
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, it is implicit but it should be GET request. Fixed in the tech spec to explicitly say this.

router.get("/static/*").handler(staticHandler);
router.get("/").handler(staticHandler); // serves index.html by default

Expand Down Expand Up @@ -307,6 +310,11 @@ BidderDetailsHandler bidderDetailsHandler(BidderCatalog bidderCatalog) {
return new BidderDetailsHandler(bidderCatalog);
}

@Bean
NotificationEventHandler eventNotificationHandler(CompositeAnalyticsReporter compositeAnalyticsReporter) {
return NotificationEventHandler.create(compositeAnalyticsReporter);
}

@Bean
StaticHandler staticHandler() {
return StaticHandler.create("static").setCachingEnabled(false);
Expand Down
13 changes: 13 additions & 0 deletions src/main/java/org/prebid/server/util/ResourceUtil.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package org.prebid.server.util;

import org.apache.commons.compress.utils.IOUtils;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStream;
Expand Down Expand Up @@ -30,4 +32,15 @@ public static String readFromClasspath(String path) throws IOException {
return reader.lines().collect(Collectors.joining("\n"));
}
}

/**
* Reads files from classpath as array of bytes. Throws {@link IllegalArgumentException} if file was not found.
*/
public static byte[] readByteArrayFromClassPath(String path) throws IOException {
final InputStream resourceAsStream = ResourceUtil.class.getClassLoader().getResourceAsStream(path);
if (resourceAsStream == null) {
throw new IllegalArgumentException(String.format("Could not find file at path: %s", path));
}
return IOUtils.toByteArray(resourceAsStream);
}
}
Binary file added src/main/resources/static/tracking-pixel.jpg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added src/main/resources/static/tracking-pixel.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
17 changes: 17 additions & 0 deletions src/test/java/org/prebid/server/ApplicationTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,11 @@
import org.prebid.server.cache.proto.response.CacheObject;
import org.prebid.server.cookie.proto.Uids;
import org.prebid.server.proto.request.CookieSyncRequest;
import org.prebid.server.proto.request.EventNotificationRequest;
import org.prebid.server.proto.response.BidderUsersyncStatus;
import org.prebid.server.proto.response.CookieSyncResponse;
import org.prebid.server.proto.response.UsersyncInfo;
import org.prebid.server.util.ResourceUtil;
import org.skyscreamer.jsonassert.JSONAssert;
import org.skyscreamer.jsonassert.JSONCompareMode;
import org.springframework.boot.test.context.SpringBootTest;
Expand Down Expand Up @@ -1694,6 +1696,21 @@ public void infoBidderDetailsShouldReturnMetadataForBidder() throws IOException
.body(Matchers.equalTo(jsonFrom("info-bidders/test-info-bidder-details-response.json")));
}

@Test
public void eventHandlerShouldRespondWithJPGTrackingPixel() throws IOException {
given(spec)
.queryParam("format", "jpg")
.body(mapper.writeValueAsString(EventNotificationRequest.builder()
.type("win").bidder("rubicon").bidId("bidId").build()))
.post("/event")
.then()
.assertThat()
.statusCode(200)
.header("content-type", "image/jpeg")
.body(Matchers.equalTo(
Buffer.buffer(ResourceUtil.readByteArrayFromClassPath("static/tracking-pixel.jpg")).toString()));
}

@Test
public void shouldAskExchangeWithUpdatedSettingsFromCache() throws IOException, JSONException {
// given
Expand Down
Loading