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

Improvements in error handling #7164

Merged
merged 6 commits into from
Dec 23, 2024
Merged

Improvements in error handling #7164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
64ed4e0
Improvements in error handling
amontenegro Dec 19, 2024
b54ce9d
Some checks should not generate logs
amontenegro Dec 23, 2024
57693ad
Remove several meaningless logs and remove the stack trace of a few
amontenegro Dec 23, 2024
583d549
Fix logic to lock records
amontenegro Dec 23, 2024
cc72703
Looks like a small delay is needed between each new entry???
amontenegro Dec 23, 2024
b7960c8
Merge branch 'main' into ImprovementsInErrorLogsAndErrorHandling
amontenegro Dec 23, 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
39 changes: 30 additions & 9 deletions orcid-api-common/src/main/java/org/orcid/api/common/exception/JSONInputValidator.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading