Skip to content

Commit

Permalink
Improvements in error handling (#7164)
Browse files Browse the repository at this point in the history 
* Improvements in error handling

* Some checks should not generate logs

* Remove several meaningless logs and remove the stack trace of a few

* Fix logic to lock records

* Looks like a small delay is needed between each new entry???
  • Loading branch information
@amontenegro
amontenegro authored Dec 23, 2024
1 parent 0685233 commit 043ec93
Show file tree
Hide file tree
Showing 10 changed files with 123 additions and 84 deletions.
39 changes: 30 additions & 9 deletions orcid-api-common/src/main/java/org/orcid/api/common/exception/JSONInputValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import javax.xml.validation.SchemaFactory;
import javax.xml.validation.Validator;

import org.apache.commons.lang3.exception.ExceptionUtils;
import org.orcid.core.exception.ApplicationException;
import org.orcid.core.exception.InvalidJSONException;
import org.slf4j.Logger;
Expand All @@ -31,7 +32,6 @@ public class JSONInputValidator {

static {
SCHEMA_LOCATIONS = new HashMap<>();


SCHEMA_LOCATIONS.put(org.orcid.jaxb.model.v3.release.record.Work.class, "/record_3.0/work-3.0.xsd");
SCHEMA_LOCATIONS.put(org.orcid.jaxb.model.v3.release.record.Funding.class, "/record_3.0/funding-3.0.xsd");
Expand Down Expand Up @@ -128,15 +128,31 @@ public void validateJSONInput(Object obj) {

try {
source = new JAXBSource(CONTEXTS.get(clazz), obj);
VALIDATORS.get(clazz).validate(source);
Validator validator = VALIDATORS.get(clazz);
if(validator != null) {
validator.validate(source);
} else {
throw new InvalidJSONException("Unable to find validator for class " + clazz.getName());
}
} catch (SAXException e) {
Map<String, String> params = new HashMap<>();
params.put("error", e.getCause().getCause().getMessage());
throw new InvalidJSONException(params);
Throwable rootCause = ExceptionUtils.getRootCause(e);
if(rootCause != null) {
throw new InvalidJSONException(rootCause.getMessage(), e);
} else {
// For SAXException, the message is usually 2 levels deep
throw new InvalidJSONException(e.getCause().getCause().getMessage(), e);
}
} catch (Exception e) {
LOGGER.error("General exception from json validator", e);
throw new ApplicationException(e);
}
LOGGER.error("Unable to find validator for class " + clazz.getName());
Map<String, String> params = new HashMap<>();
Throwable rootCause = ExceptionUtils.getRootCause(e);
if(rootCause != null) {
throw new InvalidJSONException(rootCause.getMessage(), e);
} else {
throw new InvalidJSONException(e.getMessage(), e);
}
}
}

public void validate2_1APIJSONInput(Object obj) {
Expand All @@ -153,8 +169,13 @@ public void validate2_1APIJSONInput(Object obj) {
VALIDATORS_2_1_API.get(clazz).validate(source);
} catch (SAXException e) {
Map<String, String> params = new HashMap<>();
params.put("error", e.getCause().getCause().getMessage());
throw new InvalidJSONException(params);
Throwable rootCause = ExceptionUtils.getRootCause(e);
if(rootCause != null) {
throw new InvalidJSONException(rootCause.getMessage(), e);
} else {
// For SAXException, the message is usually 2 levels deep
throw new InvalidJSONException(e.getCause().getCause().getMessage(), e);
}
} catch (Exception e) {
throw new ApplicationException(e);
}
Expand Down
69 changes: 38 additions & 31 deletions orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidExceptionMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,18 @@
import javax.annotation.Resource;
import javax.persistence.NoResultException;
import javax.servlet.http.HttpServletRequest;
import javax.ws.rs.Consumes;
import javax.ws.rs.NotFoundException;
import javax.ws.rs.Produces;
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.*;
import javax.ws.rs.core.Context;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
import javax.ws.rs.ext.ExceptionMapper;
import javax.ws.rs.ext.Provider;

import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang3.exception.ExceptionUtils;
import org.apache.solr.client.solrj.impl.HttpSolrClient.RemoteSolrException;
import org.jbibtex.TokenMgrError;
import org.orcid.api.common.exception.JSONInputValidator;
import org.orcid.api.common.filter.ApiVersionFilter;
import org.orcid.api.common.util.ApiUtils;
import org.orcid.core.api.OrcidApiConstants;
Expand All @@ -42,6 +41,7 @@
import org.springframework.context.MessageSource;
import org.springframework.security.authentication.DisabledException;
import org.springframework.security.core.AuthenticationException;
import org.springframework.security.oauth2.common.exceptions.InvalidClientException;
import org.springframework.security.oauth2.common.exceptions.OAuth2Exception;
import org.springframework.security.oauth2.common.exceptions.RedirectMismatchException;
import org.springframework.stereotype.Component;
Expand Down Expand Up @@ -88,52 +88,60 @@ public class OrcidExceptionMapper implements ExceptionMapper<Throwable> {
public Response toResponse(Throwable t) {
// Whatever exception has been caught, make sure we log it.
String clientId = securityManager.getClientIdFromAPIRequest();
if (t instanceof NotFoundException) {
logShortError(t, clientId);
} else if (t instanceof NoResultException) {
logShortError(t, clientId);
} else if (t instanceof OrcidDeprecatedException) {
logShortError(t, clientId);
} else if (t instanceof LockedException) {
logShortError(t, clientId);
} else if (t instanceof DeactivatedException) {
logShortError(t, clientId);
} else if (t instanceof DisabledException) {
logShortError(t, clientId);
if(t instanceof OrcidDeprecatedException
|| t instanceof LockedException
|| t instanceof DeactivatedException
|| t instanceof OrcidNoBioException
|| t instanceof OrcidNonPublicElementException
|| t instanceof OrcidNoResultException
|| t instanceof NoResultException
|| t instanceof NotFoundException
|| t instanceof OrcidUnauthorizedException
|| t instanceof DisabledException
|| t instanceof OrcidDuplicatedActivityException
|| t instanceof OrcidDuplicatedElementException) {
// Do not log any of these exceptions
} else if (t instanceof ClientDeactivatedException) {
logShortError(t, clientId);
} else if (t instanceof OrcidNonPublicElementException) {
logShortError(t, clientId);
} else if (t instanceof OrcidBadRequestException) {
logShortError(t, clientId);
} else if (t instanceof RedirectMismatchException) {
logShortError(t, clientId);
} else if (t instanceof DuplicatedGroupIdRecordException) {
logShortError(t, clientId);
} else if (t instanceof OrcidDuplicatedActivityException) {
logShortError(t, clientId);
} else if (t instanceof OrcidDuplicatedElementException) {
logShortError(t, clientId);
} else if (t instanceof OrcidNotificationException) {
logShortError(t, clientId);
} else if (t instanceof OrcidNoBioException) {
logShortError(t, clientId);
} else if (t instanceof RemoteSolrException) {
logShortError(t, clientId);
} else if (t instanceof WebApplicationException) {
logShortError(t, clientId);
} else if (t instanceof TokenMgrError) {
logShortError(t, clientId);
} else if (t instanceof OrcidNoResultException) {
} else if (t instanceof InvalidPutCodeException) {
logShortError(t, clientId);
} else if (t instanceof OrcidUnauthorizedException) {
} else if (t instanceof MismatchedPutCodeException) {
logShortError(t, clientId);
} else if (t instanceof InvalidPutCodeException) {
} else if (t instanceof WrongSourceException) {
logShortError(t, clientId);
} else if (t instanceof MismatchedPutCodeException) {
} else if (t instanceof OrcidWebhookNotFoundException) {
logShortError(t, clientId);
} else if(t instanceof OrcidAccessControlException) {
logShortError(t, clientId);
} else if (t instanceof InvalidJSONException) {
logShortError(t, clientId);
} else if(t instanceof StartDateAfterEndDateException) {
logShortError(t, clientId);
} else if(t instanceof InvalidClientException) {
logShortError(t, clientId);
} else if (t instanceof InvalidDisambiguatedOrgException) {
LOGGER.error("Error for client ID: " + clientId + "InvalidDisambiguatedOrgException: Disambiguated org is empty or null");
} else {
LOGGER.error("An exception has occured processing request from client " + clientId, t);
Throwable rootCause = ExceptionUtils.getRootCause(t);
if(rootCause != null) {
LOGGER.error("An exception has occurred processing request from client " + clientId, rootCause);
} else {
LOGGER.error("An exception has occurred processing request from client " + clientId, t);
}
}

if (isOAuthTokenRequest()) {
Expand Down Expand Up @@ -166,8 +174,7 @@ public Response toResponse(Throwable t) {
}

private void logShortError(Throwable t, String clientId) {
StringBuffer temp = new StringBuffer(t.getClass().getSimpleName() + " exception from client: ").append(clientId).append(". ").append(t.getMessage());
LOGGER.error(temp.toString());
LOGGER.error(t.getClass().getSimpleName() + " exception from client: " + clientId + ((t.getMessage() == null) ? "." : "." + t.getMessage()));
}

private Response oAuthErrorResponse(Throwable t) {
Expand Down
7 changes: 4 additions & 3 deletions orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidJacksonJaxbJsonProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,10 @@ public Object readFrom(Class<Object> arg0, Type arg1, Annotation[] arg2, MediaTy
try {
o = super.readFrom(arg0, arg1, arg2, arg3, arg4, arg5);
} catch (JsonMappingException e) {
Map<String, String> params = new HashMap<>();
params.put("error", e.getMessage());
throw new InvalidJSONException(params);
throw new InvalidJSONException(e.getMessage(), e);
}
if(o == null) {
throw new InvalidJSONException("Couldn't read object, data seems to be empty or invalid");
}
if (jsonInputValidator.canValidate(o.getClass())) {
RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes();
Expand Down
6 changes: 2 additions & 4 deletions ...pi-common/src/main/java/org/orcid/api/common/jaxb/OrcidJacksonJaxbJsonProviderPretty.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,8 @@ public Object readFrom(Class<Object> arg0, Type arg1, Annotation[] arg2, MediaTy
Object o = null;
try{
o = super.readFrom(arg0, arg1, arg2, arg3, arg4, arg5);
}catch(JsonMappingException e){
Map<String, String> params = new HashMap<>();
params.put("error", e.getMessage());
throw new InvalidJSONException(params);
} catch(JsonMappingException e){
throw new InvalidJSONException(e.getMessage(), e);
}
if (jsonInputValidator.canValidate(o.getClass())){
RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes();
Expand Down
12 changes: 7 additions & 5 deletions orcid-core/src/main/java/org/orcid/core/exception/InvalidJSONException.java
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package org.orcid.core.exception;

import java.util.Map;

public class InvalidJSONException extends ApplicationException {

private static final long serialVersionUID = 1L;

public InvalidJSONException(Map<String, String> params) {
super(params);

public InvalidJSONException(String message) {
super(message);
}

public InvalidJSONException(String message, Throwable cause) {
super(message, cause);
}

}
2 changes: 1 addition & 1 deletion ...src/main/java/org/orcid/core/manager/read_only/impl/ClientDetailsManagerReadOnlyImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public ClientDetailsEntity findByClientId(String clientId) {
LOGGER.error("Client getId() doesn't match. Requested: " + clientId + " Returned: " + result.getId());
}
} catch (NoResultException nre) {
LOGGER.error("Error getting client by id:" + clientId, nre);
LOGGER.error("Client not found: '" + clientId + "'");
}
return result;
}
Expand Down
2 changes: 1 addition & 1 deletion ...d-core/src/main/java/org/orcid/core/manager/read_only/impl/RecordManagerReadOnlyImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ private Preferences getPreferences(String orcid) {
}
return preferences;
} catch(Exception e) {
LOGGER.error("Exception loading preferences for " + orcid, e);
LOGGER.info("Locale for user " + orcid + " is not valid for V2.0/V2.1 API");
}
return null;
}
Expand Down
4 changes: 4 additions & 0 deletions orcid-core/src/main/java/org/orcid/core/oauth/service/OrcidOauth2TokenDetailServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import javax.annotation.Resource;
import javax.persistence.NoResultException;

import org.apache.commons.lang3.StringUtils;
import org.orcid.core.constants.RevokeReason;
import org.orcid.core.oauth.OrcidOauth2TokenDetailService;
import org.orcid.core.utils.cache.redis.RedisClient;
Expand Down Expand Up @@ -50,6 +51,9 @@ public void setOrcidOauth2TokenDetailDao(OrcidOauth2TokenDetailDao orcidOauth2To

@Override
public OrcidOauth2TokenDetail findNonDisabledByTokenValue(String token) {
if(StringUtils.isBlank(token)) {
return null;
}
try {
return orcidOauth2TokenDetailDaoReadOnly.findNonDisabledByTokenValue(token);
} catch (NoResultException e) {
Expand Down
12 changes: 8 additions & 4 deletions orcid-persistence/src/test/java/org/orcid/persistence/dao/NotificationDaoTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ public void testFindPermissionByOrcidAndClient() {
}

@Test
public void testFindLatestByOrcid() throws IllegalAccessException {
public void testFindLatestByOrcid() throws IllegalAccessException, InterruptedException {
NotificationEntity entity = notificationDao.findLatestByOrcid("0000-0000-0000-0004");
assertNull(entity);
Long lastId = null;
Expand All @@ -290,8 +290,8 @@ public void testFindLatestByOrcid() throws IllegalAccessException {
FieldUtils.writeField(newEntity, "lastModified", now, true);
newEntity.setAmendedSection(AMENDED_SECTION_UNKNOWN);
newEntity.setClientSourceId("APP-6666666666666666");
newEntity.setNotificationIntro("Intro");
newEntity.setNotificationSubject("Subject");
newEntity.setNotificationIntro("Intro " + i);
newEntity.setNotificationSubject("Subject " + i);
newEntity.setNotificationType(NOTIFICATION_TYPE_AMENDED);
newEntity.setOrcid("0000-0000-0000-0004");
newEntity.setSendable(true);
Expand All @@ -301,12 +301,16 @@ public void testFindLatestByOrcid() throws IllegalAccessException {
assertNotNull(freshFromDB);
assertNotNull(freshFromDB.getDateCreated());
assertNotNull(freshFromDB.getLastModified());
assertEquals("Intro " + i, freshFromDB.getNotificationIntro());
assertEquals("Subject " + i, freshFromDB.getNotificationSubject());
assertNotNull(newEntity.getId());
if (lastId == null) {
lastId = freshFromDB.getId();
} else {
assertTrue(lastId < freshFromDB.getId());
assertTrue(lastId + " is not less than " + freshFromDB.getId(), lastId < freshFromDB.getId());
lastId = freshFromDB.getId();
}
Thread.sleep(50);
}
}

Expand Down
Loading

0 comments on commit 043ec93

Please sign in to comment.