Handling repeated values in query parameters about aiogoogle HOT 6 CLOSED

I'll tackle the validation commits tomorrow or the day after, depending on how crazy my day gets. At the same time, I will put some thought into the rewrite structure you proposed. It'll be a few more weeks until I have enough time to actually attempt the rewrite, but that's something I'll definitely look at doing!

Until then, here's what I came up with in terms of a workaround: unfortunately, get and batchGet have a more distinct syntax than expected:
get: sheets.googleapis.com/v4/spreadsheets/{spreadsheetId}/values/{range}?optional_param=value
batchGet: sheets.googleapis.com/v4/spreadsheets/{spreadsheetId}/values:batchGet?optional_param=value&ranges={range1}&ranges={range2}
This makes the patch a little bit trickier, but not by too much:

import re
import aiogoogle
from typing import List

def _patch_batch_req(request: aiogoogle.models.Request,
                     ranges: List[str]) -> aiogoogle.models.Request:
   '''generate a spreadsheets.values.batchGet request from a 
    spreadsheets.values.get request given a list of range strings. 
    This is a workaround to circumvent an aiogoogle issue with repeated 
    query parameters, see https://github.com/omarryhan/aiogoogle/issues/52
    '''
    # this matches the get-request's range string up to and including the ? or eol
    pattern = r'\/[^?\/]*(\?|$)'
    base_url = request.url
    patched_url = re.sub(pattern, ':batchGet?', base_url)
    # if optional params are present, need to add a &
    if not patched_url.endswith('?'):
        patched_url += '&'
    patched_url += 'ranges=' + '&ranges='.join(ranges)
    request.url = patched_url
    return request

from aiogoogle.

Hey, I found a way simpler solution to this problem and I just pushed the fix.

The solution is to pass an array instead of a MultiDict. I iterated over all of Google's discovery documents and checked if Google ever asks for an actual serialized array, and as expected, they don't. So now, whenever a user passes an array, Aiogoogle would check if this parameter is repeatable. If it is, it would break them into separate URL args, instead of serializing the whole array as a JSON array. If it isn't, it's treated like before (validates with an error, and if validation is turned off, it would serialize the whole array as a JSON array). Hope I explained this well.

tl;dr you can now pass ranges=['range1', 'range2'] or ranges='range1' instead of the workaround you and I mentioned above.

from aiogoogle.

Update: I have a draft for a potential fix (untested as of writing this) sitting on my fork, but since a.) this would introduce some changes to the API and b.) I'm not entirely sure I have touched all necessary parts of the code and I don't want to inadvertently break some other parts of the library, I'd like to talk the details through with you @omarryhan before submitting the PR

from aiogoogle.

Thanks for your effort @lukasthaler

Unfortunately, I wasn't aware of the repeated attribute while writing this library. If I were to go back in time, I would've let the user pass the URL args as a normal dict instead of kwargs. That way we can make the user pass a MultiDict in case they had a repeateable argument. Plus, it's easier to manage overall and less confusing for beginners.

The easiest workaround I can think of right now is:

req = sheet.get(ranges='first:range')

req.url = req.url + '&ranges=second:range'

As to your fork, I think if we were to break the API, it's better if we go the MultiDict route because it's the standard way of passing multiple URL args in the async Python community. But that would be a huge task to rewrite, test and document. Alternatively, we can mention the workaround above in the docs so that people don't waste much time on this.

from aiogoogle.

Looking at the MultiDict, I can clearly see how that'd be the way to go, I'll definitely scrap my way of supporting repeated parameters in favor of a MultiDict approach. What about the other two commits concerning the range validator and the addition of the enum validator? Should those be merged?

Concerning the rewrite: I've always been a fan of fixing things properly and I'd be happy to contribute my part to making aiogoogle a more complete library. I'll definitely need some explaining on the inner works of things, but f you deem the rewrite worth the time, I'd love to be a part of it

from aiogoogle.

The range validator change seems like a bug fix. Sure, let's merge that.

I'll be happy to merge the enum validation feature if you can add a test or two for it.

I must say, I'm happy that people are finding the validation code useful. I added it because Google was sometimes ambigious about the details of a bad-request response. But thought that it would be nothing more than a gimmick (a buggy one in fact) for most people.

As for the rewrite, I don't think I'll be able to rewrite it myself anytime soon. But, I'll be more than happy to help you along the way should you decide to do it. Just make sure it's backward compatible. One more reason I wanted to pass URL args as a dict rather than kwargs is because it sometimes conflicts with other keywords that are used by this library, like validate.

One way I can see this issue fixed without breaking backward compatibility is by accepting a kwarg called args or url_args as well as the already existing **kwargs. But only one method should be used by the library user, otherwise it would raise an error.

If we decide to do that, args should accept both a dict or a MultiDict. If given a dict, it should convert it to a MultiDict to have a uniform API for further processing.

That's just how I would go about it, I'm open to any ideas from your side.

from aiogoogle.