Additional parameters beyond odata

[ryleyb]

I've been writing some code to retrieve schedules and found that I need to make a call like this:

params = { 'sinceModifiedTimestamp': datetime.now().date() - timedelta(days=1) }
client.get_schedule(start_date=min_date, end_date=max_date, odata_kwargs=params)

As a result, I found myself changing the pre pipeline to allow for that. I'm happy to write something up if you have a suggestion of how you want this handled. Another example in the docs is includeDeletes.

[jjorissen52]

@ryleyb Ideally, the solution is something that continues to validate parameters for each endpoint, but does not require future updates to the code if qgenda starts to support new params. I think a solution would look like this:

Add new options to the config file for each endpoint, e.g. get_schedule_params which is a comma-seperated list of parameter values accepted for the corresponding endpoint. In the settings, that would look something like:

DEFAULTS = {
  . . .
  'get_schedule_params': 'sinceModifiedTimestamp,includeDeletes,<other new params>',
  # the rest . . .
}
. . .py
DEBUG = config_dict.get('debug')
PARAMS = {
    # e.g 'get_schedule_params': {'sinceModifiedTimestamp', 'includeDeletes'},
    key: set([p.strip() for p in config_dict.get(key).split(',')])
    for key in config_dict if key.endswith('params')
}

And then we add a new filter which checks against settings.PARAMS to make sure the extras they are passing are valid.

I have made a new branch which is a functioning proof-of-concept for get_schedule. Because I no longer have access to the qgenda API, I would ask you to implement this feature yourself or continue working off of my new branch and run the tests locally when you are done (example below).

python -m unittest ./qgenda/tests

The new branch has a single new test suite (python -m unittest ./qgenda/tests/test_params.py) that I would expect to be trivially extended for each API method that you implement.

Submit a PR with a screenshot of all tests passing and we can go from there.

[ryleyb]

Awesome, thanks for doing all that. I am just getting around to doing my part (running the tests) and I found a couple issues. The first one applies to both master and extra-params, the test qgenda/tests/test_exceptions.py fails a couple tests:

• TestRaises.test_bad_login expects an APIError but gets an HTTPError
• TestNoRaise.test_bad_login throws an error:
  Traceback (most recent call last): File "/Users/ryleyb/src/python-qgenda/qgenda/tests/test_exceptions.py", line 30, in test_bad_login client.authenticate() File "/Users/ryleyb/src/python-qgenda/qgenda/pipeline/pipelines.py", line 45, in decorated return method(client_obj, **params) File "/Users/ryleyb/src/python-qgenda/qgenda/pipeline/pipelines.py", line 71, in decorated client_self, response = pipline_func(client_self, response) File "/Users/ryleyb/src/python-qgenda/qgenda/pipeline/post.py", line 20, in real_decorator setattr(response, 'text', { AttributeError: can't set attribute
  Which I can confirm happens if you just make a new requests.Response and try to set it's text attribute, it complains. I was able to get around that by setting response._context to a byte string, but maybe you know a better way. FWIW I am running requests version 2.28.1.

The more relevant problem to the issue addressed in your extra_params branch is that the extra_params persist between calls in qgenda.tests.test_get_methods.TestSchedule. When I run the unittests, it runs them in reverse order, starting with test_get_schedule_odata. The odata_kwargs given there show up in the calls to get_schedule in the subsequent tests (test_get_schedule_start_end,test_get_schedule_start_only). Running each test individually, they pass.

[jjorissen52]

@ryleyb Ah, I guess text is a property now. I don't know what possessed me to overwrite the response text, seems like a questionable decision on my part. I don't want to break backwards compatibility, but I also think it makes a lot of sense to let the user see what the request actually returned. Your solution of setting _content seems fine, but I would also like to add something like "unmodified_response" so it looks something like this:

modified_response = {
    "error": response.status_code,
    "error_description": response.reason,
    "unmodified": response.text,
}
response._content = json.dumps(modified_response).encode()
# otherwise you will be unable to read the content
response._content_consumed = False
return response

There's a risk this will break again i the future because we are using unsupported APIs for their request object, but that's the burden I apparently took on when I tried modifying the response object to begin with.

[jjorissen52]

The more relevant problem to the issue addressed in your extra_params branch is that the extra_params persist between calls in qgenda.tests.test_get_methods.TestSchedule. When I run the unittests, it runs them in reverse order, starting with test_get_schedule_odata. The odata_kwargs given there show up in the calls to get_schedule in the subsequent tests (test_get_schedule_start_end,test_get_schedule_start_only). Running each test individually, they pass.

As for this problem, I assume that has something to do with this

python-qgenda/qgenda/pipeline/pipelines.py

Lines 9 to 16 in e68db12

	class pipeline:
	def __call__(self, method):
	# so we have access to the original arguments from the function definition
	method._original_arg_names = getattr(
	method,
	'_original_arg_names',
	{*helpers.get_arg_names(method)}
	)

happening when I don't expect it to. I really can't be sure without digging in.

[ryleyb]

I'm not much of a python coder, so it's a bit beyond me unfortunately. I can confirm that pipeline.__call__ is hit twice for get_schedule. Once with ('self', 'start_date', 'end_date', 'odata_kwargs', 'headers') (via the pre_execution_pipeline.__call__) and then once with ('client_self') (via the post_execution_pipeline.__call__).

When the tests run, this is what happens in pre_execution_pipeline.decorated for the get_schedule call. When it runs the first test:

request_key = get_schedule:():{'start_date':_'08/30/2022',_'end_date':_'09/09/2022',_'odata_kwargs':_{'$_select':_'Date',_'$select':_'Date,StaffEmail',_'$orderby':_'Date',_'$filter':_"startswith(StaffEmail,_'c')"}}
params = {'start_date': '08/30/2022', 'end_date': '09/09/2022', 'odata_kwargs': {'$_select': 'Date', '$select': 'Date,StaffEmail', '$orderby': 'Date', '$filter': "startswith(StaffEmail, 'c')"}, 'headers': None}

Which I think is expected.

When it runs the 2nd test:

request_key = get_schedule:():{'start_date':_'08/30/2022',_'end_date':_'09/09/2022'}
params = {'start_date': '08/30/2022', 'end_date': '09/09/2022', 'odata_kwargs': {'$select': 'Date,StaffEmail', '$orderby': 'Date', '$filter': "startswith(StaffEmail, 'c')"}, 'headers': { ... } }

[jjorissen52]

Well, I'm at a loss for now. I might get some time to look at it after work today.

[ryleyb]

FWIW, removing the @functools.cache on the helper functions get_default_args and get_arg_names gets it working, but I don't understand the goal well enough to see if that's an acceptable solution.

[jjorissen52]

@ryleyb If that works then sounds good.