RedHatInsights / insights-api-common-rails

Header, Encryption, RBAC, Serialization, Pagination and other common behavior for Insights microservices built with Rails
Apache License 2.0
3 stars 25 forks source link

Updated the support of association filter to expect operators #176

Closed abellotti closed 4 years ago

abellotti commented 4 years ago

While this worked earlier, it was an ambiguous specification when no operators are present, i.e. filter[attr]=value case where eq is not specified.

bdunne commented 4 years ago

I think my only concern is that we're not allowing the implied eq comparator if no comparator is given and that is inconsistent between direct attributes and association attributes

abellotti commented 4 years ago

If we want to allow the implied eq with associations (which is what we have today), then the change instead would be to the specs in topo land for [attr][bad_op]=value would be seen as attr.bad_op = value instead of attr bad_op value, so changing the expected error message seen. (i.e. bad association attr instead of bad operator bad_op). Let me know @bdunne.

abellotti commented 4 years ago

@lindgrenj6 @syncrou any preference here ?

syncrou commented 4 years ago

@abellotti so are you saying the implied eq for associations is only a spec change in topo?

abellotti commented 4 years ago

correct, if we want implied eq, it's already here in common, so I'd close this PR and update specs in topo instead.

abellotti commented 4 years ago

any update on this ? should we stick with the optional eq for associations ?

lindgrenj6 commented 4 years ago

@abellotti I'm of the opinion maybe we should still allow implied eq, but that's just me. Thoughts @bdunne @syncrou?

abellotti commented 4 years ago

Closing this PR in preference for supporting the implied eq on association filters as Sources is already GA'd with this capability.