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

Memoize validate result for is_valid? as validate is destructive. #61

Closed
wants to merge 1 commit into from

Conversation

bmo
Copy link

@bmo bmo commented Jan 26, 2013

See Issue #60

@bmo
Copy link
Author

bmo commented Jan 31, 2013

Folks, not sure what to do here -- the pull request breaks a test; this broken test incrementally changes attributes on the object until the test passes, which I suppose could be a use case for the object, however not one that I would have anticipated.

The patch is meant to address the case of is_valid? being invoked again on the same object after is_valid was calculated to be true. There is no current test for that, however it is a use case we encountered (a logging statement with response.is_valid? was inserted ahead of a 2nd invocation, the logging one succeeded, the 2nd raised an exception (NoMethodError: undefined method `text' for nil:NilClass in xml_security) , and it took a while to understand why.

A much better means to resolve the situation would be to not have XMLSecurity::SignedDocument#validate_doc perform element.remove on at least two pieces of the document.

@stouset
Copy link
Contributor

stouset commented Jan 31, 2013

I think PR #63 satisfies this issue better. Rather than memoizing, it prevents the document from being changed by validation. Please let me know if I've misunderstood something and I'll be happy to reopen.

@stouset stouset closed this Jan 31, 2013
@bmo
Copy link
Author

bmo commented Jan 31, 2013

That is a much better way to do it, yes, thanks!

On Thu, Jan 31, 2013 at 11:39 AM, Stephen Touset
notifications@github.comwrote:

I think PR #63 https://github.com/onelogin/ruby-saml/issues/63satisfies this issue better. Rather than memoizing, it prevents the
document from being changed by validation. Please let me know if I've
misunderstood something and I'll be happy to reopen.


Reply to this email directly or view it on GitHubhttps://github.com//pull/61#issuecomment-12961448.

e: bmoran@onehub.com
p: +1 206 390 4376

Onehub, Inc.
www.onehub.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants