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.

Sign up for GitHub

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

Jump to bottom

Removing assertions from code and replacing with relevant exceptions #1085

Merged
merged 13 commits into from
Oct 22, 2024
Merged

Removing assertions from code and replacing with relevant exceptions #1085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
579fdc1
Remove assertions used to check argument validity in public methods
asloobq Oct 10, 2024
44093af
Exclude code and assertion from source needed only for tests
asloobq Oct 10, 2024
b91591b
replace assert for refresh token length with exception
asloobq Oct 10, 2024
0a82473
Replace assert with NPE in statsCollector public methods
asloobq Oct 10, 2024
bd6438a
Convert assert in default switch case to exception. Refactor methods
asloobq Oct 11, 2024
6e964c6
Refactor error messages to static string
asloobq Oct 11, 2024
9c24935
refactor redundant code to call function
asloobq Oct 11, 2024
60abeed
Convert asserts to exceptions in OptOut code
asloobq Oct 15, 2024
ce6bf60
[CI Pipeline] Released Snapshot version: 5.40.87-alpha-110-SNAPSHOT
Oct 15, 2024
19c6c7a
Fix partition timestamp check
asloobq Oct 16, 2024
a890446
Changed assertion to warn
asloobq Oct 16, 2024
78e61d0
Add comment for warning
asloobq Oct 17, 2024
24ab08e
Remove unwanted assertions. Address review comments
asloobq Oct 18, 2024
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
4 changes: 3 additions & 1 deletion src/main/java/com/uid2/operator/monitoring/StatsCollectorHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ public StatsCollectorHandler(IStatsCollectorQueue _statCollectorQueue, Vertx ver

@Override
public void handle(RoutingContext routingContext) {
assert routingContext != null;
if (routingContext == null) {
throw new NullPointerException();
}

//setAuthClient() has not yet been called, so getAuthClient() would return null. This is resolved by using addBodyEndHandler()
routingContext.addBodyEndHandler(v -> addStatsMessageToQueue(routingContext));
Expand Down
2 changes: 0 additions & 2 deletions src/main/java/com/uid2/operator/monitoring/StatsCollectorVerticle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,6 @@ public void handleMessage(Message message) {
return;
}

assert messageItem != null;

String path = messageItem.getPath();
String apiVersion = "v0";
String endpoint = path.substring(1);
Expand Down
14 changes: 3 additions & 11 deletions src/main/java/com/uid2/operator/service/UIDOperatorService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ public class UIDOperatorService implements IUIDOperatorService {
private final Duration refreshIdentityAfter;

private final OperatorIdentity operatorIdentity;
private final TokenVersion tokenVersionToUseIfNotV4;
private final int advertisingTokenV4Percentage;
private final Set<Integer> siteIdsUsingV4Tokens;
protected final TokenVersion tokenVersionToUseIfNotV4;
protected final int advertisingTokenV4Percentage;
protected final Set<Integer> siteIdsUsingV4Tokens;
private final TokenVersion refreshTokenVersion;
private final boolean identityV3Enabled;

Expand Down Expand Up @@ -319,12 +319,4 @@ private GlobalOptoutResult getGlobalOptOutResult(UserIdentity userIdentity, bool
Instant result = this.optOutStore.getLatestEntry(userIdentity);
return new GlobalOptoutResult(result);
}

public TokenVersion getAdvertisingTokenVersionForTests(int siteId) {
assert this.advertisingTokenV4Percentage == 0 || this.advertisingTokenV4Percentage == 100; //we want tests to be deterministic
if (this.siteIdsUsingV4Tokens.contains(siteId)) {
return TokenVersion.V4;
}
return this.advertisingTokenV4Percentage == 100 ? TokenVersion.V4 : this.tokenVersionToUseIfNotV4;
}
}
7 changes: 6 additions & 1 deletion src/main/java/com/uid2/operator/service/V2RequestUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ public static void handleRefreshTokenInResponseBody(JsonObject bodyJson, KeyMana
.appendInt(refreshKey.getId())
.appendBytes(encrypted)
.getBytes());
assert modifiedToken.length() == V2_REFRESH_PAYLOAD_LENGTH;
if (modifiedToken.length() != V2_REFRESH_PAYLOAD_LENGTH) {
final String errorMsg = "Generated refresh token's length=" + modifiedToken.length()
+ " is not equal to=" + V2_REFRESH_PAYLOAD_LENGTH;
LOGGER.error(errorMsg);
throw new IllegalArgumentException(errorMsg);
}

bodyJson.put("refresh_token", modifiedToken);
bodyJson.put("refresh_response_key", refreshResponseKey);
Expand Down
41 changes: 32 additions & 9 deletions src/main/java/com/uid2/operator/store/CloudSyncOptOutStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,11 @@ private IndexUpdateMessage getIndexUpdateMessage(Instant now, Collection<String>
ium.addDeltaFile(f);
else if (OptOutUtils.isPartitionFile(f))
ium.addPartitionFile(f);
else assert false;
else {
final String errorMsg = "File to index " + f + " is not of type delta or partition";
LOGGER.error(errorMsg);
throw new IllegalStateException(errorMsg);
}
}

Collection<String> indexedNonSynthetic = indexedFiles.stream()
Expand All @@ -538,7 +542,12 @@ else if (OptOutUtils.isPartitionFile(f))

Instant tsOld = OptOutUtils.lastPartitionTimestamp(indexedNonSynthetic);
Instant tsNew = OptOutUtils.lastPartitionTimestamp(newNonSynthetic);
assert tsOld == Instant.EPOCH || tsNew == Instant.EPOCH || tsOld.isBefore(tsNew);
if (tsOld != Instant.EPOCH && tsNew != Instant.EPOCH && !tsOld.isBefore(tsNew)) {
final String errorMsg = "Last partition timestamp of indexed files " + tsOld.getEpochSecond()
+ " is after last partition of non-indexed files " + tsNew.getEpochSecond();
LOGGER.error(errorMsg);
throw new IllegalStateException(errorMsg);
}
// if there are new partitions in this update, let index delete some in-mem delta caches that is old
if (tsNew != Instant.EPOCH) {
tsNew = tsNew.minusSeconds(fileUtils.lookbackGracePeriod());
Expand Down Expand Up @@ -594,15 +603,21 @@ private OptOutStoreSnapshot updateIndexInternal(IndexUpdateContext iuc) {
try {
if (numPartitions == 0) {
// if update doesn't have a new partition, simply update heap with new log data
assert iuc.getDeltasToRemove().size() == 0;
if (!iuc.getDeltasToRemove().isEmpty()) {
final String errorMsg = "Invalid number of Deltas to remove=" + iuc.getDeltasToRemove().size()
+ " when there are 0 new partitions to index";
LOGGER.error(errorMsg);
throw new IllegalStateException(errorMsg);
}
return this.processDeltas(iuc);
} else if (numPartitions > 1) {
// should not load more than 1 partition at a time, unless during service bootstrap
assert this.iteration == 0;
if (this.iteration != 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcollins-ttd FYI

final String errorMsg = "Should not load more than 1 partition at a time, unless during service bootstrap. Current iteration " + this.iteration;
// Leaving this as a warning as this condition is true in production
LOGGER.warn(errorMsg);
}
return this.processPartitions(iuc);
} else {
// array size cannot be a negative value
assert numPartitions == 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this condition is always true

return this.processPartitions(iuc);
}
} finally {
Expand All @@ -628,7 +643,11 @@ private OptOutStoreSnapshot processDeltasImpl(IndexUpdateContext iuc) {
// this is thread-safe, as heap is not being used
// and bloomfilter can tolerate false positive
for (byte[] data : loadedData) {
assert data.length != 0;
if (data.length == 0) {
final String errorMsg = "Loaded delta file has 0 size";
LOGGER.error(errorMsg);
throw new IllegalStateException(errorMsg);
}

OptOutCollection newLog = new OptOutCollection(data);
this.heap.add(newLog);
Expand Down Expand Up @@ -679,7 +698,11 @@ private OptOutStoreSnapshot processPartitionsImpl(IndexUpdateContext iuc) {
}
for (String key : sortedPartitionFiles) {
byte[] data = iuc.loadedPartitions.get(key);
assert data.length != 0;
if (data.length == 0) {
final String errorMsg = "Loaded partition file has 0 size";
LOGGER.error(errorMsg);
throw new IllegalStateException(errorMsg);
}
newPartitions[snapIndex++] = new OptOutPartition(data);
}

Expand Down
12 changes: 7 additions & 5 deletions src/main/java/com/uid2/operator/util/Tuple.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package com.uid2.operator.util;

import java.util.Objects;

public class Tuple {
public static class Tuple2<T1, T2> {
private final T1 item1;
private final T2 item2;

public Tuple2(T1 item1, T2 item2) {
assert item1 != null;
assert item2 != null;
Objects.requireNonNull(item1);
Objects.requireNonNull(item2);

this.item1 = item1;
this.item2 = item2;
Expand All @@ -34,9 +36,9 @@ public static class Tuple3<T1, T2, T3> {
private final T3 item3;

public Tuple3(T1 item1, T2 item2, T3 item3) {
assert item1 != null;
assert item2 != null;
assert item3 != null;
Objects.requireNonNull(item1);
Objects.requireNonNull(item2);
Objects.requireNonNull(item3);

this.item1 = item1;
this.item2 = item2;
Expand Down
60 changes: 25 additions & 35 deletions src/main/java/com/uid2/operator/vertx/UIDOperatorVerticle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
import io.vertx.core.json.JsonArray;
import io.vertx.core.json.JsonObject;
import io.vertx.ext.web.AllowForwardHeaders;
import io.vertx.ext.web.Route;
import io.vertx.ext.web.Router;
import io.vertx.ext.web.RoutingContext;
import io.vertx.ext.web.handler.BodyHandler;
Expand Down Expand Up @@ -129,6 +128,10 @@ public class UIDOperatorVerticle extends AbstractVerticle {
//"Android" is from https://github.com/IABTechLab/uid2-android-sdk/blob/ff93ebf597f5de7d440a84f7015a334ba4138ede/sdk/src/main/java/com/uid2/UID2Client.kt#L46
//"ios"/"tvos" is from https://github.com/IABTechLab/uid2-ios-sdk/blob/91c290d29a7093cfc209eca493d1fee80c17e16a/Sources/UID2/UID2Client.swift#L36-L38
private final static List<String> SUPPORTED_IN_APP = Arrays.asList("Android", "ios", "tvos");

private static final String ERROR_INVALID_INPUT_WITH_PHONE_SUPPORT = "Required Parameter Missing: exactly one of [email, email_hash, phone, phone_hash] must be specified";
private static final String ERROR_INVALID_INPUT_EMAIL_MISSING = "Required Parameter Missing: exactly one of email or email_hash must be specified";
private static final String ERROR_INVALID_INPUT_EMAIL_TWICE = "Only one of email or email_hash can be specified";
public final static String ORIGIN_HEADER = "Origin";

public UIDOperatorVerticle(JsonObject config,
Expand Down Expand Up @@ -450,7 +453,7 @@ else if(emailHash != null) {
input = InputUtil.normalizePhoneHash(phoneHash);
}

if (!checkForInvalidTokenInput(input, rc)) {
if (!isTokenInputValid(input, rc)) {
return;
}

Expand Down Expand Up @@ -869,7 +872,7 @@ private void handleTokenRefreshV2(RoutingContext rc) {
private void handleTokenValidateV1(RoutingContext rc) {
try {
final InputUtil.InputVal input = this.phoneSupport ? getTokenInputV1(rc) : getTokenInput(rc);
if (!checkForInvalidTokenInput(input, rc)) {
if (!isTokenInputValid(input, rc)) {
return;
}
if ((Arrays.equals(ValidateIdentityForEmailHash, input.getIdentityInput()) && input.getIdentityType() == IdentityType.Email)
Expand Down Expand Up @@ -900,7 +903,7 @@ private void handleTokenValidateV2(RoutingContext rc) {
final JsonObject req = (JsonObject) rc.data().get("request");

final InputUtil.InputVal input = getTokenInputV2(req);
if (!checkForInvalidTokenInput(input, rc)) {
if (!isTokenInputValid(input, rc)) {
return;
}
if ((input.getIdentityType() == IdentityType.Email && Arrays.equals(ValidateIdentityForEmailHash, input.getIdentityInput()))
Expand Down Expand Up @@ -932,17 +935,13 @@ private void handleTokenGenerateV1(RoutingContext rc) {
try {
final InputUtil.InputVal input = this.phoneSupport ? this.getTokenInputV1(rc) : this.getTokenInput(rc);
platformType = getPlatformType(rc);
if (!checkForInvalidTokenInput(input, rc)) {
return;
} else {
if (isTokenInputValid(input, rc)) {
final IdentityTokens t = this.idService.generateIdentity(
new IdentityRequest(
new PublisherIdentity(siteId, 0, 0),
input.toUserIdentity(this.identityScope, 1, Instant.now()),
OptoutCheckPolicy.defaultPolicy()));

//Integer.parseInt(rc.queryParam("privacy_bits").get(0))));

ResponseUtil.Success(rc, toJsonV1(t));
recordTokenResponseStats(siteId, TokenResponseStatsCollector.Endpoint.GenerateV1, TokenResponseStatsCollector.ResponseStatus.Success, siteProvider, t.getAdvertisingTokenVersion(), platformType);
}
Expand All @@ -959,9 +958,7 @@ private void handleTokenGenerateV2(RoutingContext rc) {
platformType = getPlatformType(rc);

final InputUtil.InputVal input = this.getTokenInputV2(req);
if (!checkForInvalidTokenInput(input, rc)) {
return;
} else {
if (isTokenInputValid(input, rc)) {
final String apiContact = getApiContact(rc);

switch (validateUserConsent(req)) {
Expand All @@ -978,8 +975,9 @@ private void handleTokenGenerateV2(RoutingContext rc) {
break;
}
default: {
assert false : "Please update UIDOperatorVerticle.handleTokenGenerateV2 when changing UserConsentStatus";
break;
final String errorMsg = "Please update UIDOperatorVerticle.handleTokenGenerateV2 when changing UserConsentStatus";
LOGGER.error(errorMsg);
throw new IllegalStateException(errorMsg);
}
}

Expand Down Expand Up @@ -1037,7 +1035,7 @@ private void handleTokenGenerate(RoutingContext rc) {
final InputUtil.InputVal input = this.getTokenInput(rc);
Integer siteId = null;
if (input == null) {
SendClientErrorResponseAndRecordStats(ResponseStatus.ClientError, 400, rc, "Required Parameter Missing: exactly one of email or email_hash must be specified", siteId, TokenResponseStatsCollector.Endpoint.GenerateV0, TokenResponseStatsCollector.ResponseStatus.BadPayload, siteProvider, TokenResponseStatsCollector.PlatformType.Other);
SendClientErrorResponseAndRecordStats(ResponseStatus.ClientError, 400, rc, ERROR_INVALID_INPUT_EMAIL_MISSING, siteId, TokenResponseStatsCollector.Endpoint.GenerateV0, TokenResponseStatsCollector.ResponseStatus.BadPayload, siteProvider, TokenResponseStatsCollector.PlatformType.Other);
return;
}
else if (!input.isValid()) {
Expand All @@ -1053,8 +1051,6 @@ else if (!input.isValid()) {
input.toUserIdentity(this.identityScope, 1, Instant.now()),
OptoutCheckPolicy.defaultPolicy()));

//Integer.parseInt(rc.queryParam("privacy_bits").get(0))));

recordTokenResponseStats(siteId, TokenResponseStatsCollector.Endpoint.GenerateV0, TokenResponseStatsCollector.ResponseStatus.Success, siteProvider, t.getAdvertisingTokenVersion(), TokenResponseStatsCollector.PlatformType.Other);
sendJsonResponse(rc, toJson(t));

Expand Down Expand Up @@ -1234,7 +1230,7 @@ private void handleBucketsV2(RoutingContext rc) {

private void handleIdentityMapV1(RoutingContext rc) {
final InputUtil.InputVal input = this.phoneSupport ? this.getTokenInputV1(rc) : this.getTokenInput(rc);
if (!checkForInvalidTokenInput(input, rc)) {
if (!isTokenInputValid(input, rc)) {
return;
}
try {
Expand All @@ -1254,13 +1250,7 @@ private void handleIdentityMap(RoutingContext rc) {
final InputUtil.InputVal input = this.getTokenInput(rc);

try {
if (input == null) {
ResponseUtil.ClientError(rc, "Required Parameter Missing: exactly one of email or email_hash must be specified");
}
else if (!input.isValid()) {
ResponseUtil.ClientError(rc, "Invalid email or email_hash");
}
else {
if (isTokenInputValid(input, rc)) {
final Instant now = Instant.now();
final MappedIdentity mappedIdentity = this.idService.map(input.toUserIdentity(this.identityScope, 0, now), now);
rc.response().end(EncodingUtils.toBase64String(mappedIdentity.advertisingId));
Expand Down Expand Up @@ -1363,9 +1353,9 @@ private InputUtil.InputVal getTokenInputV1(RoutingContext rc) {
return null;
}

private boolean checkForInvalidTokenInput(InputUtil.InputVal input, RoutingContext rc) {
Copy link
Contributor

Choose a reason for hiding this comment

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

good that you renamed checkForInvalidTokenInput as after reading the original codes and what bool it returns, this method name is highly deceptive!

private boolean isTokenInputValid(InputUtil.InputVal input, RoutingContext rc) {
if (input == null) {
String message = this.phoneSupport ? "Required Parameter Missing: exactly one of [email, email_hash, phone, phone_hash] must be specified" : "Required Parameter Missing: exactly one of email or email_hash must be specified";
String message = this.phoneSupport ? ERROR_INVALID_INPUT_WITH_PHONE_SUPPORT : ERROR_INVALID_INPUT_EMAIL_MISSING;
ResponseUtil.ClientError(rc, message);
return false;
} else if (!input.isValid()) {
Expand All @@ -1381,11 +1371,11 @@ private InputUtil.InputVal[] getIdentityBulkInput(RoutingContext rc) {
final JsonArray emailHashes = obj.getJsonArray("email_hash");
// FIXME TODO. Avoid Double Iteration. Turn to a decorator pattern
if (emails == null && emailHashes == null) {
ResponseUtil.ClientError(rc, "Exactly one of email or email_hash must be specified");
ResponseUtil.ClientError(rc, ERROR_INVALID_INPUT_EMAIL_MISSING);
return null;
} else if (emails != null && !emails.isEmpty()) {
if (emailHashes != null && !emailHashes.isEmpty()) {
ResponseUtil.ClientError(rc, "Only one of email or email_hash can be specified");
ResponseUtil.ClientError(rc, ERROR_INVALID_INPUT_EMAIL_TWICE);
return null;
}
return createInputList(emails, false);
Expand All @@ -1398,7 +1388,7 @@ private InputUtil.InputVal[] getIdentityBulkInput(RoutingContext rc) {
private InputUtil.InputVal[] getIdentityBulkInputV1(RoutingContext rc) {
final JsonObject obj = rc.body().asJsonObject();
if(obj.isEmpty()) {
ResponseUtil.ClientError(rc, "Exactly one of [email, email_hash, phone, phone_hash] must be specified");
ResponseUtil.ClientError(rc, ERROR_INVALID_INPUT_WITH_PHONE_SUPPORT);
return null;
}
final JsonArray emails = JsonParseUtils.parseArray(obj, "email", rc);
Expand Down Expand Up @@ -1430,7 +1420,7 @@ private InputUtil.InputVal[] getIdentityBulkInputV1(RoutingContext rc) {
}

if (validInputs == 0 || nonEmptyInputs > 1) {
ResponseUtil.ClientError(rc, "Exactly one of [email, email_hash, phone, phone_hash] must be specified");
ResponseUtil.ClientError(rc, ERROR_INVALID_INPUT_WITH_PHONE_SUPPORT);
return null;
}

Expand Down Expand Up @@ -1511,9 +1501,9 @@ private void handleIdentityMapV2(RoutingContext rc) {
final InputUtil.InputVal[] inputList = getIdentityMapV2Input(rc);
if (inputList == null) {
if (this.phoneSupport)
ResponseUtil.ClientError(rc, "Exactly one of [email, email_hash, phone, phone_hash] must be specified");
ResponseUtil.ClientError(rc, ERROR_INVALID_INPUT_WITH_PHONE_SUPPORT);
else
ResponseUtil.ClientError(rc, "Required Parameter Missing: exactly one of email or email_hash must be specified");
ResponseUtil.ClientError(rc, ERROR_INVALID_INPUT_EMAIL_MISSING);
return;
}

Expand Down Expand Up @@ -1579,11 +1569,11 @@ private void handleIdentityMapBatch(RoutingContext rc) {
final JsonArray emails = obj.getJsonArray("email");
final JsonArray emailHashes = obj.getJsonArray("email_hash");
if (emails == null && emailHashes == null) {
ResponseUtil.ClientError(rc, "Exactly one of email or email_hash must be specified");
ResponseUtil.ClientError(rc, ERROR_INVALID_INPUT_EMAIL_MISSING);
return;
} else if (emails != null && !emails.isEmpty()) {
if (emailHashes != null && !emailHashes.isEmpty()) {
ResponseUtil.ClientError(rc, "Only one of email or email_hash can be specified");
ResponseUtil.ClientError(rc, ERROR_INVALID_INPUT_EMAIL_TWICE);
return;
}
inputList = createInputList(emails, false);
Expand Down
Loading