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.

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

DomXmlMessageValidator accepts any payload for an empty control message #422

Closed
bjoerndev opened this issue Jun 26, 2018 · 4 comments
Closed

Comments

@bjoerndev
Copy link

In a complex special case we get an unexpected behavior of citrus XML comparison. I debugged into citrus and came to a line where I'm not sure if this is ok:

In DomXmlMessageValidator.validateMessageContent (at the moment line 342) [1] there is a
} else if (!StringUtils.hasText(controlMessagePayload)) { return; }
which causes that in case of an empty control message any payload gets accepted because the comparison ends without exception. I would expect throwing an exception or having an assert instead of the return.

Thank you for your great work and for checking this line.

[1]

@christophd
Copy link
Member

Empty control message is an indicator that you are not interested in validating the message payload at all. This skips the payload message validation. This is a convention decision we made ages ago. I am not 100% happy with this either but it is the convention.

In case you need to really validate for an empty message payload you should use one of the validation matchers as expected payload e.g. @empty()@

@christophd christophd added this to the SOMEDAY milestone Jul 10, 2018
@svettwer
Copy link
Contributor

Hi @bjoerndev!

Did @christophd work for you or do you have any further questions?

BR,
Sven

@svettwer svettwer removed this from the SOMEDAY milestone Jul 20, 2018
@bjoerndev
Copy link
Author

Hi @svettwer ,

thanks for looking at this.
For me there are no open tasks or questions: We have our own class derived from the original citrus validator where we put some special extensions. There I added that this case isn't allowed, and everything is fine for us now.
But general for Citrus I think this could be an unexpected and therefore dangerous behavior. So if there is time, I suggest changing this, or to add an explicit option to configure this.

Greetings, Björn

@svettwer
Copy link
Contributor

Hi @bjoerndev,

yeah you're right. One should rethink this convention. Unfortunately this convention is not easy to change and there is actually no concrete requirement. Therefore we won't change this short term. Nevertheless, we'll keep this in mind in case that we've time for some major refactoring tasks.

BR,
Sven

@svettwer svettwer removed the READY label Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants