sni / lmd

Livestatus Multitool Daemon - Create livestatus federation from multiple sources
https://labs.consol.de/omd/packages/lmd/
GNU General Public License v3.0
42 stars 31 forks source link

Do not ignore Limit: 0 #54

Closed jacobbaungard closed 5 years ago

jacobbaungard commented 5 years ago

Livestatus respond to a query with 'Limit: 0', by providing all the columns for the object type, but no actual objects.

Before this commit, LMD would ignore Limit: 0 and return all the found objects.

This commit ensures that setting Limit: 0, results in LMD responding only with the object type columns.

Signed-off-by: Jacob Hansen jhansen@op5.com

sni commented 5 years ago

looks good so far, could you adopt https://github.com/sni/lmd/blob/master/lmd/peer.go#L2571 and https://github.com/sni/lmd/blob/master/lmd/peer.go#L2586 as well. there is no need to fetch all data and remove it later completly.

jacobbaungard commented 5 years ago

Hmm, adding '>=' to L2586 and L2701 causes tests to fail:

--- FAIL: TestMainFunc (10.18s)
panic: backend never came online [recovered]
panic: backend never came online
sni commented 5 years ago

the problem here is, that in golang integer have a default value of 0. So if anyone simply creates a Request object by &Request{} he will get Limit:0 which is most likely not intended and the oposite of how it worked previously. So either everybody has to be super careful when initializing a new Request Object, which probably won't work, or we have to either invert the logic, ex.: Limit:0 is translated to -1 and only in that cases a limit of zero is applied, but thats kind of counterintuitive, or we make Request.Limit a integeger pointer instead of an integer so it can be null and this allows us to distinguish between unset and defined values. The only disadvantage with that is that we have to always test Limit != nil before using it to avoid null pointer panics. Still sounds like the best solution to me. And at least we will get compiler errors when using Request.Limit directly as integer, so we know which places to change.

jacobbaungard commented 5 years ago

Ah, just updated the PR with the requested optimization.

Changing limit to an integer pointer is probably safer though.

sni commented 5 years ago

that optimization is still right and needed :-)

jacobbaungard commented 5 years ago

Updated to use int pointer.

sni commented 5 years ago

perfect, thanks