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

Fixing problem: validation errors ignored on redirects. Closes #476 #725

Merged
merged 21 commits into from
Aug 30, 2014
Merged

Fixing problem: validation errors ignored on redirects. Closes #476 #725

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
cff51ec
Fixing problem: validation ignored on redirects.
renanigt Aug 2, 2014
c96221c
Importing the correct IllegalStateException.
renanigt Aug 2, 2014
f543fa9
Fixing indentation.
renanigt Aug 5, 2014
07912a9
Checking for validator erros on method use from Result.
renanigt Aug 6, 2014
64c3c07
Minor refactoring.
renanigt Aug 6, 2014
7a2b82b
Adding classes that handles errors.
renanigt Aug 7, 2014
7205e22
Minor.
renanigt Aug 7, 2014
c41ff72
Merge with branch master
renanigt Aug 20, 2014
3a287f1
Merge branch 'master' into validationIgnoredOnRedirects
renanigt Aug 21, 2014
117aac0
Removing unnecessary components since now we have Messages component.
renanigt Aug 21, 2014
b9614f9
Adding feature to handle errors.
renanigt Aug 25, 2014
f2af267
Undo changes on VRaptor.java
renanigt Aug 25, 2014
274adc9
Minor fixing.
renanigt Aug 26, 2014
b89218e
Minor
renanigt Aug 26, 2014
c31c1b5
Improving based on Turini's suggestion.
renanigt Aug 26, 2014
cb49d2b
Fixing a typo.
renanigt Aug 26, 2014
fa16133
Adding hadnleErrors just in instanceFor method call
renanigt Aug 26, 2014
7b7f243
Checking if Severity is ERROR
renanigt Aug 27, 2014
9869b7c
Improvements based on Lucas's suggestions.
renanigt Aug 29, 2014
ae22e99
Improvements based on Lucas's suggestion.
renanigt Aug 29, 2014
bb4c60a
Improving tests.
renanigt Aug 29, 2014
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
12 changes: 10 additions & 2 deletions vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import br.com.caelum.vraptor.View;
import br.com.caelum.vraptor.interceptor.TypeNameExtractor;
import br.com.caelum.vraptor.ioc.Container;
import br.com.caelum.vraptor.validator.Messages;

/**
* A basic implementation of a Result
Expand All @@ -48,28 +49,34 @@ public class DefaultResult extends AbstractResult {
private final Container container;
private final ExceptionMapper exceptions;
private final TypeNameExtractor extractor;
private final Messages messages;

private Map<String, Object> includedAttributes;
private boolean responseCommitted = false;


/**
* @deprecated CDI eyes only
*/
protected DefaultResult() {
this(null, null, null, null);
this(null, null, null, null, null);
}

@Inject
public DefaultResult(HttpServletRequest request, Container container, ExceptionMapper exceptions, TypeNameExtractor extractor) {
public DefaultResult(HttpServletRequest request, Container container, ExceptionMapper exceptions, TypeNameExtractor extractor,
Messages messages) {
this.request = request;
this.container = container;
this.extractor = extractor;
this.includedAttributes = new HashMap<>();
this.exceptions = exceptions;
this.messages = messages;
}

@Override
public <T extends View> T use(Class<T> view) {
messages.assertAbsenceOfErrors();

responseCommitted = true;
return container.instanceFor(view);
}
Expand Down Expand Up @@ -107,4 +114,5 @@ public Result include(Object value) {
String key = extractor.nameFor(value.getClass());
return include(key, value);
}

}
52 changes: 20 additions & 32 deletions vraptor-core/src/main/java/br/com/caelum/vraptor/observer/ExecuteMethod.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,27 +17,29 @@

package br.com.caelum.vraptor.observer;

import static org.slf4j.LoggerFactory.getLogger;

import java.lang.reflect.Method;

import javax.enterprise.context.Dependent;
import javax.enterprise.event.Event;
import javax.enterprise.event.Observes;
import javax.inject.Inject;

import net.vidageek.mirror.dsl.Mirror;
import net.vidageek.mirror.exception.ReflectionProviderException;

import org.slf4j.Logger;

import br.com.caelum.vraptor.InterceptionException;
import br.com.caelum.vraptor.controller.ControllerMethod;
import br.com.caelum.vraptor.core.MethodInfo;
import br.com.caelum.vraptor.events.InterceptorsExecuted;
import br.com.caelum.vraptor.events.MethodExecuted;
import br.com.caelum.vraptor.events.MethodReady;
import br.com.caelum.vraptor.interceptor.ApplicationLogicException;
import br.com.caelum.vraptor.validator.Messages;
import br.com.caelum.vraptor.validator.ValidationException;
import br.com.caelum.vraptor.validator.Validator;
import net.vidageek.mirror.dsl.Mirror;
import net.vidageek.mirror.exception.ReflectionProviderException;
import org.slf4j.Logger;

import javax.enterprise.context.Dependent;
import javax.enterprise.event.Event;
import javax.enterprise.event.Observes;
import javax.inject.Inject;
import java.lang.reflect.Method;

import static br.com.caelum.vraptor.view.Results.nothing;
import static org.slf4j.LoggerFactory.getLogger;

/**
* Interceptor that executes the logic method.
Expand All @@ -52,7 +54,7 @@ public class ExecuteMethod {
private final static Logger log = getLogger(ExecuteMethod.class);

private final MethodInfo methodInfo;
private final Validator validator;
private final Messages messages;

private Event<MethodExecuted> methodExecutedEvent;
private Event<MethodReady> methodReady;
Expand All @@ -65,10 +67,10 @@ protected ExecuteMethod() {
}

@Inject
public ExecuteMethod(MethodInfo methodInfo, Validator validator,
public ExecuteMethod(MethodInfo methodInfo, Messages messages,
Event<MethodExecuted> methodExecutedEvent, Event<MethodReady> methodReady) {
this.methodInfo = methodInfo;
this.validator = validator;
this.messages = messages;
this.methodExecutedEvent = methodExecutedEvent;
this.methodReady = methodReady;
}
Expand All @@ -84,22 +86,8 @@ public void execute(@Observes InterceptorsExecuted event) {
Object instance = event.getControllerInstance();
Object result = new Mirror().on(instance).invoke().method(reflectionMethod).withArgs(parameters);

if (validator.hasErrors()) { // method should have thrown
// ValidationException
if (log.isDebugEnabled()) {
try {
validator.onErrorUse(nothing());
} catch (ValidationException e) {
log.debug("Some validation errors occured: {}", e.getErrors());
}
}
throw new InterceptionException(
"There are validation errors and you forgot to specify where to go. Please add in your method "
+ "something like:\n"
+ "validator.onErrorUse(page()).of(AnyController.class).anyMethod();\n"
+ "or any view that you like.\n"
+ "If you didn't add any validation error, it is possible that a conversion error had happened.");
}
messages.assertAbsenceOfErrors();

this.methodInfo.setResult(result);
methodExecutedEvent.fire(new MethodExecuted(method, methodInfo));
} catch (IllegalArgumentException e) {
Expand Down
2 changes: 1 addition & 1 deletion vraptor-core/src/main/java/br/com/caelum/vraptor/validator/DefaultValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ public <T extends View> T onErrorUse(Class<T> view) {
outjector.outjectRequestMap();

logger.debug("there are errors on result: {}", getErrors());
return viewsFactory.instanceFor(view, getErrors());
return viewsFactory.instanceFor(view, messages.handleErrors());
}

@Override
Expand Down
53 changes: 53 additions & 0 deletions vraptor-core/src/main/java/br/com/caelum/vraptor/validator/Messages.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,20 @@
*/
package br.com.caelum.vraptor.validator;

import static br.com.caelum.vraptor.view.Results.nothing;
import static org.slf4j.LoggerFactory.getLogger;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import javax.enterprise.context.RequestScoped;
import javax.inject.Inject;
import javax.inject.Named;

import org.slf4j.Logger;

/**
* Managed class that stores all application messages like errors, warnings and info. This
* class is useful to display messages categorized by severity in your view. To choose a severity
Expand All @@ -40,10 +46,29 @@
@RequestScoped
public class Messages {

private final static Logger log = getLogger(Messages.class);

private Map<Severity, List<Message>> messages = new HashMap<>();
private boolean unhandledErrors = false;
private final Validator validator;

/**
* @deprecated CDI eyes only.
*/
protected Messages() {
this(null);
}

@Inject
public Messages(Validator validator) {
this.validator = validator;
}

public Messages add(Message message) {
get(message.getSeverity()).add(message);
if(Severity.ERROR.equals(message.getSeverity())) {
unhandledErrors = true;
}
return this;
}

Expand Down Expand Up @@ -83,4 +108,32 @@ public List<Message> getAll() {
private MessageList createMessageList() {
return new MessageList(new ArrayList<Message>());
}

public List<Message> handleErrors() {
unhandledErrors = false;
return getErrors();
}

public boolean hasUnhandledErrors() {
return unhandledErrors;
}

public void assertAbsenceOfErrors() {
if (hasUnhandledErrors()) {
if (log.isDebugEnabled()) {
try {
validator.onErrorUse(nothing());
} catch (ValidationException e) {
log.debug("Some validation errors occured: {}", e.getErrors());
}
}
Copy link
Member

Choose a reason for hiding this comment

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

you don't need validator here.

you already have the errors (getErrors()), just use it.

and remove validator dependency.

no more cyclic dependency ;)

Copy link
Member

Choose a reason for hiding this comment

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

you can change this whole if on line 123 to:

log.debug("Some validation errors occured: {}", getErrors());

slf4j already does the if internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lucascs what about validator.onErrorUse(nothing()); ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True @lucascs, now I see your point. 😃

Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

it's from the time we didn't have validator.getErrors() method, so we had to catch the exception to be able to get the errors. VRaptor 3 inheritance ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot @lucascs. 😃

throw new IllegalStateException(
"There are validation errors and you forgot to specify where to go. Please add in your method "
+ "something like:\n"
+ "validator.onErrorUse(page()).of(AnyController.class).anyMethod();\n"
+ "or any view that you like.\n"
+ "If you didn't add any validation error, it is possible that a conversion error had happened.");
}
}

}
30 changes: 20 additions & 10 deletions vraptor-core/src/test/java/br/com/caelum/vraptor/core/DefaultResultTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import br.com.caelum.vraptor.interceptor.TypeNameExtractor;
import br.com.caelum.vraptor.ioc.Container;
import br.com.caelum.vraptor.proxy.WeldProxifier;
import br.com.caelum.vraptor.validator.Messages;
import br.com.caelum.vraptor.view.DefaultHttpResultTest.RandomController;
import br.com.caelum.vraptor.view.LogicResult;
import br.com.caelum.vraptor.view.PageResult;
Expand All @@ -50,28 +51,30 @@ public class DefaultResultTest {
@Mock private HttpServletRequest request;
@Mock private Container container;
@Mock private TypeNameExtractor extractor;
@Mock private Messages messages;

private Result result;
private WeldProxifier weldProxifier;

@Before
public void setup() {
MockitoAnnotations.initMocks(this);
result = new DefaultResult(request, container, null, extractor);
result = new DefaultResult(request, container, null, extractor, messages);
weldProxifier = new WeldProxifier();
}

public static class MyView implements View {

}

@Test
public void shouldUseContainerForNewView() {
final MyView expectedView = new MyView();
when(container.instanceFor(MyView.class)).thenReturn(expectedView);

MyView view = result.use(MyView.class);
assertThat(view, is(expectedView));
verify(messages).assertAbsenceOfErrors();
}

@Test
Expand All @@ -90,6 +93,7 @@ public void shouldDelegateToPageResultOnForwardToURI() throws Exception {
result.forwardTo("/any/uri");

verify(pageResult).forwardTo("/any/uri");
verify(messages).assertAbsenceOfErrors();
}

@Test
Expand All @@ -100,6 +104,7 @@ public void shouldDelegateToPageResultOnRedirectToURI() throws Exception {
result.redirectTo("/any/uri");

verify(pageResult).redirectTo("/any/uri");
verify(messages).assertAbsenceOfErrors();
}

private <T extends View> T mockResult(Class<T> view) {
Expand All @@ -117,6 +122,7 @@ public void shouldDelegateToPageResultOnPageOf() throws Exception {
result.of(RandomController.class);

verify(pageResult).of(RandomController.class);
verify(messages).assertAbsenceOfErrors();
}
@Test
public void shouldDelegateToLogicResultOnForwardToLogic() throws Exception {
Expand All @@ -136,7 +142,7 @@ public void shouldDelegateToLogicResultOnRedirectToLogic() throws Exception {
result.redirectTo(RandomController.class);

verify(logicResult).redirectTo(RandomController.class);

verify(messages).assertAbsenceOfErrors();
}
@Test
public void shouldDelegateToLogicResultOnRedirectToLogicWithInstance() throws Exception {
Expand All @@ -146,7 +152,7 @@ public void shouldDelegateToLogicResultOnRedirectToLogicWithInstance() throws Ex
result.redirectTo(new RandomController());

verify(logicResult).redirectTo(RandomController.class);

verify(messages).assertAbsenceOfErrors();
}

@Test
Expand All @@ -157,8 +163,9 @@ public void shouldDelegateToLogicResultOnForwardToLogicWithInstance() throws Exc
result.forwardTo(new RandomController());

verify(logicResult).forwardTo(RandomController.class);

verify(messages).assertAbsenceOfErrors();
}

@Test
public void shouldDelegateToPageResultOnPageOfWithInstance() throws Exception {

Expand All @@ -167,7 +174,7 @@ public void shouldDelegateToPageResultOnPageOfWithInstance() throws Exception {
result.of(new RandomController());

verify(pageResult).of(RandomController.class);

verify(messages).assertAbsenceOfErrors();
}

@Test
Expand All @@ -177,6 +184,7 @@ public void shouldDelegateToLogicResultOnRedirectToLogicWithProxifiedTypeInstanc
assertThat(randomProxy, instanceOf(ProxyObject.class));
result.redirectTo(randomProxy);
verify(logicResult).redirectTo(RandomController.class);
verify(messages).assertAbsenceOfErrors();
}

@Test
Expand All @@ -186,6 +194,7 @@ public void shouldDelegateToLogicResultOnForwardToLogicWithProxifiedTypeInstance
assertThat(randomProxy, instanceOf(ProxyObject.class));
result.forwardTo(randomProxy);
verify(logicResult).forwardTo(RandomController.class);
verify(messages).assertAbsenceOfErrors();
}

@Test
Expand All @@ -195,6 +204,7 @@ public void shouldDelegateToPageResultOnPageOfWithProxifiedTypeInstance() throws
assertThat(randomProxy, instanceOf(ProxyObject.class));
result.of(randomProxy);
verify(logicResult).of(RandomController.class);
verify(messages).assertAbsenceOfErrors();
}

@Test
Expand All @@ -205,7 +215,7 @@ public void shouldDelegateToStatusOnNotFound() throws Exception {
result.notFound();

verify(status).notFound();

verify(messages).assertAbsenceOfErrors();
}

@Test
Expand All @@ -216,7 +226,7 @@ public void shouldDelegateToStatusOnPermanentlyRedirectToUri() throws Exception
result.permanentlyRedirectTo("url");

verify(status).movedPermanentlyTo("url");

verify(messages).assertAbsenceOfErrors();
}

@Test
Expand All @@ -227,7 +237,7 @@ public void shouldDelegateToStatusOnPermanentlyRedirectToControllerClass() throw
result.permanentlyRedirectTo(RandomController.class);

verify(status).movedPermanentlyTo(RandomController.class);

verify(messages).assertAbsenceOfErrors();
}

@Test
Expand All @@ -238,7 +248,7 @@ public void shouldDelegateToStatusOnPermanentlyRedirectToControllerInstance() th
result.permanentlyRedirectTo(new RandomController());

verify(status).movedPermanentlyTo(RandomController.class);

verify(messages).assertAbsenceOfErrors();
Copy link
Member

Choose a reason for hiding this comment

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

leave this verify only for the method which is checking this behavior. No need to repeat it on all.

}


Expand Down
Loading