fix: Fix matomo api client to support POST for authen in Matomo 5 #664
Conversation
anuunchin
left a comment
anuunchin
left a comment
There was a problem hiding this comment.
Pretty cool! Left a few small questions/comments here and there. Did you write local tests for the matomo 5? Feel free to add them to the PR as well :)
| """ | ||
|
|
||
| def __init__(self, api_token: str, url: str) -> None: | ||
| def __init__(self, api_token: str, url: str, call_method: str = "GET") -> None: |
There was a problem hiding this comment.
Literal["GET", "POST"] = "GET" might be a better (narrower) option
|
|
||
| self.base_url = url | ||
| self.auth_token = api_token | ||
| if call_method.upper() not in ["GET", "POST"]: |
There was a problem hiding this comment.
If we use Literal, we can drop the .upper() part
| Args: | ||
| params (DictStrAny): Parameters for the API request. | ||
| base_params (DictStrAny): Parameters for the API request. | ||
| detailed_params (DictStrAny): Detailed parameters for the API request. |
There was a problem hiding this comment.
It would make sense to explain why we separate the two here (i.e. Matomo 5 requires detailed params)
There was a problem hiding this comment.
I'm thinking that detailed_params (and variables named the same way) is a bit vague, would there be a better option?
There was a problem hiding this comment.
Already added the explanation, naming convention is not really my expertise... actually I felt something off with this name too, but could not think of any better name, please feel free to recommend one
| ) | ||
| else: | ||
| headers = {"Content-type": "application/json"} | ||
| final_params = base_params.copy() |
There was a problem hiding this comment.
Wondering if there's a reason to copy the base params, maybe it's cleaner to simply
params = {**base_params, **detailed_params}
response = client.get(url=url, headers=headers, params=params)
| url (str): URL of the Matomo website. | ||
| call_method (str): HTTP method for API calls, related to authentication, | ||
| either "GET" or "POST". Default is "GET" to continue the support for Matomo 4 and below, | ||
| for Matomo 5, "POST" is recommended |
There was a problem hiding this comment.
Would be great if the thing with Only allow secure requests is mentioned here
|
|
||
| while True: | ||
| detailed_params = { | ||
| "urls[0]": f"method={method}&idSite={site_id}&filter_limit={rows_per_page}&filter_offset={filter_offset}", | ||
| "token_auth": self.auth_token, | ||
| } | ||
| detailed_params.update(extra_params) | ||
| response_data = self._request( | ||
| base_params=base_params, detailed_params=detailed_params | ||
| ) | ||
| if not response_data or not isinstance(response_data, list): | ||
| break | ||
| batch = response_data[0] if len(response_data) > 0 else [] | ||
| if not batch: | ||
| break | ||
| yield batch | ||
| filter_offset += len(batch) |
There was a problem hiding this comment.
wondering if keeping the previous flow an option here, since it was lightly easier to read
There was a problem hiding this comment.
During my test, the old flow did not work
|
Pushed the new changes, already run "make lint", all passed For the local tests, actually you must have a Matomo account, and in my case, I integrate these changes into my full pipeline, so short answer: I do not have any single unit test to test this single PR |
3 participants
Tell us what you do here
Short description
Related Issues
Additional Context