Skip to content

Conversation

dcorbacho
Copy link
Contributor

Any other value will break the validation cycle and leave the rest of the arguments without validate

Reported in the mailing list: https://groups.google.com/forum/#!topic/rabbitmq-users/7mwL2AWwUyI

Types of Changes

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

Further Comments

If this is a relatively large or complex change, kick off the discussion by
explaining why you chose the solution you did and what alternatives you
considered, etc.

Any other value will break the validation cycle and leave the rest
of the arguments without validate
[#165689664]
@dcorbacho dcorbacho requested a review from hairyhum May 2, 2019 15:35
validate_policy(KeyList) ->
case proplists:lookup(<<"queue-master-locator">> , KeyList) of
{_, Strategy} -> validate_strategy(Strategy);
{_, Strategy} -> case validate_strategy(Strategy) of
Copy link
Collaborator

Choose a reason for hiding this comment

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

I must be missing something, but I don't see how validate_strategy/1 can return anything other than {ok, _} or {error, "~p invalid queue-master-locator value", [Strategy]}

Copy link
Contributor

Choose a reason for hiding this comment

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

validate_policy/1 should return either ok or {error, term()}, but not {ok, term()}. https://github.com/rabbitmq/rabbitmq-server/blob/master/src/rabbit_policy.erl#L528

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see now, thanks.

Copy link
Contributor

@hairyhum hairyhum left a comment

Choose a reason for hiding this comment

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

This seems like fixing the original issue.
Can you add a test case in policy validation to cover that?

@dcorbacho
Copy link
Contributor Author

@hairyhum Test just added

@michaelklishin michaelklishin merged commit 3cfcacb into master May 2, 2019
@michaelklishin michaelklishin deleted the queue-master-location-validation branch May 2, 2019 23:55
michaelklishin added a commit that referenced this pull request May 3, 2019
Policy validation must return 'ok' or an error

(cherry picked from commit 3cfcacb)
@michaelklishin
Copy link
Collaborator

Backported to v3.7.x.

@michaelklishin michaelklishin changed the title Policy validation must return 'ok' or an error Policy validation could let invalid values in May 3, 2019
@michaelklishin michaelklishin added this to the 3.7.15 milestone May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants