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

Sensor "list_values" method limited to 100 keystore values #5097

Closed
jamesdreid opened this issue Nov 30, 2020 · 5 comments · Fixed by #5176
Closed

Sensor "list_values" method limited to 100 keystore values #5097

jamesdreid opened this issue Nov 30, 2020 · 5 comments · Fixed by #5176

Comments

@jamesdreid
Copy link

SUMMARY

The ST2 common "list_

STACKSTORM VERSION

st2 3.2.0, on Python 3.6.8

OS, environment, install method

Post what OS you are running this on, along with any other relevant information/

Centos8 - one-liner install

Steps to reproduce the problem

Implement a simple sensor that calls the "list_values(local=True, prefix=None)" method to retrieve the data from a keystore that contains more than 100 items.

Expected Results

Method should return all values from the keystore, regardless of the number of items or should include a limit option to select the number of results to be returned.

Actual Results

List of values returned by the method is limited to 100 due to the default value applied by the API.

@arm4b
Copy link
Member

arm4b commented Nov 30, 2020

Thanks for the report!
Confirming it sounds like a bug and the method should return all possible results.


Just for reference, list_values method described in the st2docs:
https://docs.stackstorm.com/sensors.html#list-values-local-true-prefix-none

@jamesdreid
Copy link
Author

Still looking at the code, but it appears that the sensor is calling the list_items method here:

list_items method

If I am on the right path, I can probably get a fix in place and a PR submitted on this issue fairly quickly.

@blag
Copy link
Contributor

blag commented Nov 30, 2020

You are on the right path. A PR to fix this would be very welcome.

Just a heads up though, our tests right now will fail for Python 2-related reasons, so you'll want to track #5095 and update your branch from that once that's merged.

@Kami
Copy link
Member

Kami commented Dec 12, 2020

As far as the implementation goes - depending on the interface we want expose to the sensors, we will still likely want to implement some kind of pagination.

IIRC, there is an upper limit on the largest page size in the API which can be requested via query param and even if user could request all the items at once, it probably wouldn't be a good idea (it could be slow).

One possible approach is to have two methods - list_values (which works in the same manner as the current implementation, but also takes pagination related arguments) and iterate_values (which implements iterator / generator interface and transparently handles pagination).

@jamesdreid
Copy link
Author

Took me a while to understand how the "get_all" method was implemented, but I think I finally understand and agree with the idea of a "pagination" process. My original thought was to just add the "limit" option to the "list_values" method with a default value of 100 which would match how the system functions today. I assumed anyone could update their sensor code with a "-1" value specified in the "limit" field which would match what is available to the CLI client and have it return all the records, but maybe in this case, the "-1" would enable the "pagination" code you suggest and return all the matching records from the keystore in a more resource friendly way.

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

Successfully merging a pull request may close this issue.

4 participants