googleads / googleads-java-lib

Google Ad Manager SOAP API Client Library for Java
Apache License 2.0
226 stars 360 forks source link

Replacing dfp-axis with dfp-appengine: Exception Class isn't Throwable #249

Closed galacticgumshoe closed 1 year ago

galacticgumshoe commented 1 year ago

I'm following the workaround described in issue #240 to avoid being flagged by my security teams as vulnerable to CVE-2019-0227 and other medium-level vulnerabilities. Namely, I've replaced the dependency on dfp-axis with dfp-appengine since it appears you are not planning to upgrade the transitive dependency on axis to axis2.

Aside from a completely different package structure, and the use of Lists instead of Arrays, I'm encountering a kind of significant issue with exception handling. In dfp-axis I could catch an ApiException in a try-catch block, because it was an extension ultimately of a base class that extended Throwable. The ApiException class in dfp-appengine doesn't appear to be an extension of Throwable, and can't be caught. This is useful in my ForecastService calls in order to pull out the Array of ApiError classes (now a List of ApiError) so that I could determine whether the corresponding error was one of EXCEEDED_QUOTA, INTERNAL_ERROR, SERVER_NOT_AVAILABLE (from the ForecastErrorReason enum) in order to do a backOff and retry later or fail the attempt. I'm on v202211 and here's a snippet of my catch block which the compiler errors on due to this ApiException class not Throwable:

catch (ApiException ae) {
              boolean saveErrorAndExit;
              List<ApiError> errors = ae.getErrors();
              for (ApiError error: errors) {
                  String errorMessage = error.getErrorString();
                  String trigger = error.getTrigger();

                  if (error instanceof ForecastError) {
                      ForecastErrorReason forecastErrorReason = ((ForecastError) error).getReason();
                      if (forecastErrorReason.equals(ForecastErrorReason.EXCEEDED_QUOTA) ||
                          forecastErrorReason.equals(ForecastErrorReason.INTERNAL_ERROR) ||
                          forecastErrorReason.equals(ForecastErrorReason.SERVER_NOT_AVAILABLE)) {
                          log.error("Will retry after some time");
                          saveErrorAndExit = false;
                      } else {
                          saveErrorAndExit = true;
                      }
                  } else if (error instanceof ServerError || error instanceof QuotaError || error instanceof InternalApiError) {
                      log.error("Will retry after some time");
                      saveErrorAndExit = false;
                  } else {
                      saveErrorAndExit = true;
                  }
                  if (saveErrorAndExit) {
                      if (deliveryForecastOptions != null && prospectiveLineItemIds != null && trigger != null) {
                          int index = Integer.parseInt(trigger);
                          long lineItemId = prospectiveLineItemIds[index];
                          log.error("Line Item ID {} failed with error: {}", lineItemId, errorMessage);
                          GamForecastStateId id = repository.initializeId(lineItemId, pipeLineRunId);
                          repository.updateFinalStates(List.of(id), null, errorMessage, pipeLineRunId);
                          prospectiveLineItemIds = ArrayUtils.remove(prospectiveLineItemIds, index);
                      } else {
                          throw new GamException(errorMessage);
                      }
                  }
              }
          }
galacticgumshoe commented 1 year ago

Update: I discovered there is a compiled class called ApiException_Exception which is Throwable. I've used that in the catch block and have tested that I am seeing the expected behavior when handling exceptions and retrying. Seems a little odd the use of Upper Camel and Snake Case for this class, and I can't really find it in the source code or the API documentation. Still, seems to function the same as ApiException from the dfp-axis jar, so will close this issue.