Updated RetrieveDailyCost method to handle pagination. Uses nextLink from API response.

[DanielSpindler83]

I was using the daily cost component with --dimension ResourceId and noticed that it can handle max 5000 rows as this is the max for a single API call.

For this single method I have added support for obtaining the full dataset by following the nextlink returned in the response until empty\null.

I find this useful, as combined with the time frame function my results are often more than 5000 rows.

Please let me know what you think and if you want I can add the same logic to the other calls.

Thank you for this great tool!

[welcome]

Thanks for opening this pull request!

[mivano]

Thanks, that looks like a nice addition! We need it in all the places where there is paging support. Maybe even in a more generic way; a mapper that knows how to convert it to the target format and a generic reader that knows how to handle the next link. There is already a lot of duplication.

[DanielSpindler83]

Ok, leave it with me and I'll see what I can come up with. Good points.

[DanielSpindler83]

Ok, so I looked at a few different approaches and whilst this new one may not be the most elegant, the main advantage was that it results in almost zero change to the caller methods.

Firstly, I created an overload of ExecuteCallToCostApi that doesn't except a payload - for our get requests.
Cutover the three or so methods with no payload. Tested these.
I feel this is the best approach for reducing complexity, increasing readability and providing clear signals on future use.

Secondly, I thought it best to ensure the callers of ExecuteCallToCostApi with a payload could call in the same manner post pagination changes (for me less change in those areas means I don't have to delve into them too much and less chance of breaking changes).
The biggest challenge is stitching together the response rows across paginated requests and melding back into a HttpResponseMessage, which I feel is not particularly ideal, but I got it to work. The alternative was significant changes to callers across many methods and perhaps logic to stitch rows together in a list outside of the main Api call logic.
Another core aim, given the project has been running without paginated calls up until now, was to quickly return if there no need for any further calls to get more pages.

I then proceeded to refactor the new version of ExecuteCallToCostApi with helper methods to try and reduce complexity and increase readability.

Please let me know what you think. I am still learning in many areas....

I did perform a reasonable number of tests to cover paginated and non-paginated requests across commands, however it may be useful to conduct further testing if possible.
Perhaps this project could benefit from some mock Api response testing so we can be super confident of behaviour when making changes. This is an area I may be able to dive in and help out with (maybe we pop an issue?).

[mivano]

Thanks for your ideas. I was trying to get my head around it as well; minimizing changes but still supporting paging.

I believe there are three kinds of calls in the class (which is most likely too large as well):

1. calls to retrieve the result of the query endpoint
2. calls to retrieve another known type (like a subscription object)
3. calls that just need to parse the raw message (like budgets, which might also be changed to use number 2)

The first one always has a known return type: QueryResult
This type has a properties class with the nextlink. So that is the one we need to follow and fill.

So in hindsight; the ExecuteCallToCostApi function is too technical; it already calls a specific endpoint, the result is known, the json deserialization is not needed at the caller side. That is what we run into now; the callers do the deserialization, so only there you have the nextLink.

I took a slightly different approach in this PR #147: the newly introduced ExecutePagedCallToCostApi returns a QueryResponse. This means that the callers no longer need to do deserialization, making their calls easier:

- var response = await ExecuteCallToCostApi(includeDebugOutput, payload, uri);
- 
-  CostQueryResponse? content = await response.Content.ReadFromJsonAsync<CostQueryResponse>();

+ var content = await ExecutePagedCallToCostApi(includeDebugOutput, payload, uri);

For the typed calls (like fetching the subscriptions), I have a ExecuteTypedCallToCostApi which only does a simple deserialization.

And then I still have the old skool ExecuteCallToCostApi function which returns the httpmessage. Meaning the caller needs to do the parsing. I think this one should go at some point as it should be mapped to a json object anyway.

But the ExecutePagedCallToCostApi now contains the logic to actually fetch the next page.

The PR #147 is just a draft, I have not done any extensive testing, just want to get your feedback on this idea.

Your remark about tests; yes that is a big miss here. Mostly because there will need to be a lot of mocking, but that should not be an excuse.

[DanielSpindler83]

Closing
Refer PR #147