Repository: karlicoss/promnesia (language: python)

Issue:
Handle query parameters without values
Right now, pages on isfdb like `http://www.isfdb.org/cgi-bin/title.cgi?2172` are canonicalized to `http://www.isfdb.org/cgi-bin/title.cgi`, which removes important info from the URL.

I made a somewhat hacky solution, but there is perhaps a more elegant way. The solution is in two parts:

First, modify `Spec` to accept `qkeep=True`, which is taken to mean "retain all query parameters", and have isfdb.org use that option.

```
@@ -105,11 +105,13 @@ default_qkeep = [
 
 # TODO perhaps, decide if fragment is meaningful (e.g. wiki) or random sequence of letters?
 class Spec(NamedTuple):
-    qkeep  : Optional[Collection[str]] = None
+    qkeep  : Optional[Union[Collection[str], bool]] = None
     qremove: Optional[Set[str]] = None
     fkeep  : bool = False
 
     def keep_query(self, q: str) -> Optional[int]: # returns order
+        if self.qkeep is True:
+            return 1
         qkeep = {
             q: i for i, q in enumerate(chain(default_qkeep, self.qkeep or []))
         }
@@ -183,6 +185,7 @@ specs: Dict[str, Spec] = {
     'ycombinator.com'    : S(qkeep={'id'}), # todo just keep id by default?
     'play.google.com'    : S(qkeep={'id'}),
     'answers.yahoo.com'  : S(qkeep={'qid'}),
+    'isfdb.org': S(qkeep=True),
 }
 _def_spec = S()
```

Second, pass, `keep_blank_values=True` to `parse_qsl`.

```
@@ -271,7 +274,7 @@ def transform_split(split: SplitResult):
     netloc = canonify_domain(split.netloc)
 
     path     = split.path
-    qparts   = parse_qsl(split.query)
+    qparts   = parse_qsl(split.query, keep_blank_values=True)
 
     fragment = split.fragment
 
@@ -319,7 +322,7 @@ def transform_split(split: SplitResult):
             to = to + ('', )
 
         (netloc, path, qq) = [t.format(**gd) for t in to]
-        qparts.extend(parse_qsl(qq)) # TODO hacky..
+        qparts.extend(parse_qsl(qq, keep_blank_values=True)) # TODO hacky..
         # TODO eh, qparts should really be a map or something...
         break
```

This achieves the desired result, but it's unpleasantly hacky.

First problem: this will not guarantee the order of query parameters. But is that important? Why doesn't the code just always alphabetize them? There's a note for youtube that "order matters here", but it seems to me that youtube doesn't care what order the parameters are in.

Second problem: maybe you usually don't want empty parameters kept. It could complicate things to add logic around this, but maybe something like a `keep_blank_values` property on Spec would do the job.

Third problem: it's not great to have to dig around in the program code to fix this behavior. Maybe it could be patched in from my `config.py`, once the supporting code is in place, which something like:

```python
from promnesia.cannon import specs, S
specs['isfdb.org'] = S(qkeep=True)
```

Really, all of `cannon.py` is a bit intimidating to modify (and, incidentally, shouldn't it be `canon.py`?). I'm not sure what's the right direction to go.


Discussion / hints:
Hi!
Thanks so much for diving into it and trying to figure out, really appreciated! :)

First addressing your specific fix: 

`qkeep=True` -- yeah I guess this looks good for now. Could make it `qkeep='*'` or something like that, but practically speaking it's equivalent (especially if there is a unit test, so it's easy to refactor later)

And now the questions: 

> This achieves the desired result, but it's unpleasantly hacky

Yep, agree. The whole thing is very messy (as you noticed!), 
[I have a bunch of ideas]( https://beepb00p.xyz/exobrain/projects/cannon.html#ds) about alternative implementations etc, so it's easier to write the rules. But haven't had much time to think more

> There's a note for youtube that "order matters here", but it seems to me that youtube doesn't care what order the parameters are in.

Ah sorry -- guess the comment was a bit misleading -- it matters not for youtube, but for Promnesia.

https://github.com/karlicoss/promnesia/blob/90ba0d15ac7d04b365363821adb0f722534086f3/tests/cannon.py#L31-L34

Promnesia detects whether urls are related just by looking if one is a prefix of another. For example, if we didn't reorder the parameters in `youtube.com/watch?t=491s&v=1NHbPN9pNPM`, then `youtube.com/watch?v=1NHbPN9pNPM` would be unrelated to it (because it's not a prefix). 
This matching could/should be smarter, but it is what it is atm, mainly because it's easy and fast to check if something is a prefix of a URL in an sqlite database, but proper matching would require some sort of pre-computations/caching in database in order to be fast.

> maybe you usually don't want empty parameters kept

Yeah in general makes sense (although I didn't see too many of them). Although I'm a bit confused -- isn't `2172` exactly an empty parameter in your example which you want to keep? 

> Third problem: it's not great to have to dig around in the program code to fix this behavior

yes agree, or ideally without having to change python code at all, e.g. maybe one could do it from the browser
but making it easier from the config would be a good intermediate compromise. Although would be nice to figure out an easier way to write rules in the first place, otherwise it would be hard to maintain backwards compatibility with people's configs

> and, incidentally, shouldn't it be canon.py?

Yep, but it was kind of a pun because I was thinking it would be useful to extract in a separate package later and name it `cannon` since it was free on PyPi unlike `canon`. But I checked pypi now and someone has already created `cannon` :( So perhaps will need a different name again (but it's a problem for later :) )
Okay, if this seems fine to you then I'll make up a PR later.

> Ah sorry -- guess the comment was a bit misleading -- it matters not for youtube, but for Promnesia.

Thanks for the explanation. This makes sense.

> Yeah in general makes sense (although I didn't see too many of them). Although I'm a bit confused -- isn't 2172 exactly an empty parameter in your example which you want to keep?

Right, that's what I want in my case, but I meant that changing it like I did is going to make it keep empty parameters (if they also pass qkeep) for all URLs, so it's potentially changing the behavior for URLs that people currently have working correctly. Where possible, I'd obviously prefer to isolate changes from affecting existing correct behavior (e.g. the way that adding support for `qkeep=True` can't affect any existing configs), but this change doesn't do that.
> so it's potentially changing the behavior for URLs that people currently have working correctly

Ah right -- I'd say don't worry about it too much, empty parameters like this are fairly rare in the first place. And it's not like the current implementation is always correct anyway, so it's sort of work in progress and will change
But if you can come up with some widely used sites that do rely on it, even posting them here as templates for future unit tests, would be useful!

Fix patch:
diff --git a/src/promnesia/cannon.py b/src/promnesia/cannon.py
index a3a303f2..b76cb9f0 100755
--- a/src/promnesia/cannon.py
+++ b/src/promnesia/cannon.py
@@ -105,11 +105,13 @@ def canonify_domain(dom: str) -> str:
 
 # TODO perhaps, decide if fragment is meaningful (e.g. wiki) or random sequence of letters?
 class Spec(NamedTuple):
-    qkeep  : Optional[Collection[str]] = None
+    qkeep  : Optional[Union[Collection[str], bool]] = None
     qremove: Optional[Set[str]] = None
     fkeep  : bool = False
 
     def keep_query(self, q: str) -> Optional[int]: # returns order
+        if self.qkeep is True:
+            return 1
         qkeep = {
             q: i for i, q in enumerate(chain(default_qkeep, self.qkeep or []))
         }
@@ -183,6 +185,7 @@ def make(cls, **kwargs) -> 'Spec':
     'ycombinator.com'    : S(qkeep={'id'}), # todo just keep id by default?
     'play.google.com'    : S(qkeep={'id'}),
     'answers.yahoo.com'  : S(qkeep={'qid'}),
+    'isfdb.org': S(qkeep=True),
 }
 
 _def_spec = S()
@@ -271,7 +274,7 @@ def transform_split(split: SplitResult):
     netloc = canonify_domain(split.netloc)
 
     path     = split.path
-    qparts   = parse_qsl(split.query)
+    qparts   = parse_qsl(split.query, keep_blank_values=True)
 
     fragment = split.fragment
 
@@ -319,7 +322,7 @@ def iter_rules():
             to = to + ('', )
 
         (netloc, path, qq) = [t.format(**gd) for t in to]
-        qparts.extend(parse_qsl(qq)) # TODO hacky..
+        qparts.extend(parse_qsl(qq, keep_blank_values=True)) # TODO hacky..
         # TODO eh, qparts should really be a map or something...
         break
 


Test patch:
diff --git a/tests/cannon.py b/tests/cannon.py
index d76693bc..5cd6ef50 100644
--- a/tests/cannon.py
+++ b/tests/cannon.py
@@ -294,3 +294,19 @@ def test_error():
     with pytest.raises(CanonifyException):
         # borrowed from https://bugs.mageia.org/show_bug.cgi?id=24640#c7
         canonify('https://example.com\uFF03@bing.com')
+
+@pytest.mark.parametrize("url,expected", [
+    ('https://news.ycombinator.com/item?id=', 'news.ycombinator.com/item?id='),
+    ('https://www.youtube.com/watch?v=hvoQiF0kBI8&list&index=2',
+     'youtube.com/watch?v=hvoQiF0kBI8&list='),
+])
+def test_empty_query_parameter(url, expected):
+    assert canonify(url) == expected
+
+@pytest.mark.parametrize("url,expected", [
+    ('http://www.isfdb.org/cgi-bin/title.cgi?2172', 'isfdb.org/cgi-bin/title.cgi?2172='),
+    ('http://www.isfdb.org/cgi-bin/title.cgi?2172+1', 'isfdb.org/cgi-bin/title.cgi?2172%201='),
+    ('http://www.isfdb.org/cgi-bin/title.cgi?2172&foo=bar&baz&quux', 'isfdb.org/cgi-bin/title.cgi?2172=&baz=&foo=bar&quux='),
+])
+def test_qkeep_true(url, expected):
+    assert canonify(url) == expected