GET /v1/objects/*: handle "attrs":[] as expected#8169
Conversation
BeforeAfter |
Al2Klimov
force-pushed
the
bugfix/object-query-all-attrs-8167
branch
from
dbc1de9 to
242b3f7
Compare
julianbrost
left a comment
julianbrost
left a comment
There was a problem hiding this comment.
PR works as advertised, but is this a real issue? This might break compatibility in edge-cases, so I'd avoid doing so unless needed. Docs don't look like they state anything about what happens with an empty list. @N-o-X Any second opinions?
|
Which edge-cases e.g.? |
|
Just in general if you have some code that sometimes filters fields and until now used an empty list to get all fields until now. You easily get something like this when using Go's JSON marshaling, so that's why I was thinking of this. So if we do this, this shouldn't be backported to old releases. |
|
|
@cla-bot check |
... i.e. yield no attrs and not all. refs #8167
Al2Klimov
force-pushed
the
bugfix/object-query-all-attrs-8167
branch
from
242b3f7 to
e18c923
Compare
oxzi
left a comment
oxzi
left a comment
There was a problem hiding this comment.
Generally speaking, this change is kinda trivial and should resolve the linked issue.
Even if I can only think about constructed scenarios where one explicitly wants an empty attrslist, the documentation describes this parameter as "[l]imited attribute list in the output".
Anyway, I think @julianbrost made a valid point in the prior comment, #8169 (comment). One can think of an API-facing script like the following, which would break.
def get_attrs():
''' Required attrs based on something™, but can be empty. '''
return []
# return ['name', 'address']
attrs = []
attrs.extend(get_attrs())But it is questionable how many scripts may be out there dynamically creating an attrs list.
Thus, I would be in favor for this PR but only if it appears in the next non-patch release (currently 2.15) without backports and a descriptive entry in the changelog.
|
Guess what relied on that behavior: Icinga/icingadb-web#1192 😄 |
|
Don't worry, they handle this! |
But was this change worth it to break Icinga DB Web? |
|
Yes. What did they even expect from explicitly passing an empty array?? |
The phrasing of the question and the use of multiple punctuation marks convey an attitude that is inappropriate. The behavior in question is not clearly documented anywhere, and Icinga previously worked this way. The recent change is now being used as justification to pick on the Web code across several discussions. Stop making such a fuss. And to be frank, if the documentation states that the parameter limits the attributes, then specifying an empty array does not limit the attributes. Regardless of how the fix is handled in Web, the documentation should clearly state what is to be expected.
Of course not, all this is constructed work with zero benefit. |
5 participants
... i.e. yield no attrs and not all.
fixes #8167