Empty Label response can possibly be cached from GitHub?

[miketaylr]

Sadly our existing labels tests aren't good enough. This came up trying to get #712 working.

Right now on production if you open the label gear you get an empty label w/ the following error in the console: no element found

Looks like we're sending an empty response?

This doesn't happen in dev, but the other bug is that if you open the label editor on an issue with labels, the checkboxes aren't selected. That's a regression, likely from all the namespace mangling.

I'm beginning to want to rip out all the special namespace handling code and just show them everywhere.

@karlcow any good reasons to not show browser-foo and status-foo in the webcompat UI? Or were we just trying to have a seamless transition?

[miketaylr]

I'm going to remove the tests that aren't great, IMO. We should add back the following once we solve the bug:

      'Add and remove a label': function () {
        return this.remote
          .setFindTimeout(intern.config.wc.pageLoadTimeout)
          .get(require.toUrl(url('/issues/2')))
          .findByCssSelector('.LabelEditor-launcher').click()
          .end()
          .findByCssSelector('label.LabelEditor-item input[name="contactready"]').click()
          .end()
          .sleep(200)
          .findByCssSelector('.Label--badge').getVisibleText()
          .then(function (labelText) {
            assert.include(labelText, 'contactready', 'Label appears once it has been selected');
          })
          .end()
          .findByCssSelector('.LabelEditor-launcher').click()
          .end()
          .findByCssSelector('label.LabelEditor-item input[name="contactready"]').click()
          .end()
          .findByCssSelector('.LabelEditor-btn').click()
          .end()
          .sleep(200)
          .findByCssSelector('.Label--badge')
          .getVisibleText()
          .then(function (labelText) {
            assert.include(labelText, 'contactready', 'Label has been removed');
          })
          .end();
      }

We can also add python unit tests at the API level around adding and removing labels. That can be handled in #396.

[miketaylr]

It seems like if we move to a vertical label editor (cf. #545), having the extra status- or browser- text won't be a big deal.

[miketaylr]

Hmm, the "no element found" bug doesn't reproduce for me all of the sudden. But the un-checked label checkbox reproduces 100%.

Screenshot to prove I'm not crazy:

[karlcow]

@miketaylr

any good reasons to not show browser-foo and status-foo in the webcompat UI? Or were we just trying to have a seamless transition?

The namespace is to allow us to separate them in the UI. Basically at the end I don't think the label gear (in the future) would have any kind of status- (just browser- etc). Note also that if you rip the namespace, you will get a mix-order of things. It's already messy currently. Without the namespace it will be a big bag of keywords. Not fun.

[karlcow]

Right now on production if you open the label gear you get an empty label w/ the following error in the console: no element found

This was happening to me before the renaming, specifically in Japan. When you open the gear, and github doesn't have time to send the response, and you start playing with it and it erases all labels.

[miketaylr]

Note also that if you rip the namespace, you will get a mix-order of things. It's already messy currently. Without the namespace it will be a big bag of keywords. Not fun.

Ah, OK. So we're relying on the namespace to alphabetize/sort the labels for us?

[miketaylr]

This was happening to me before the renaming, specifically in Japan.

OK, let's ignore that problem for now and focus on:

the other bug is that if you open the label editor on an issue with labels, the checkboxes aren't selected.

[miketaylr]

And now it doesn't reproduce in production or development.

So apologies @karlcow for insinuating you/we broke something here. 🙇

My best guess is something bad happening between GitHub and us. We do cache the labels endpoint, so one theory is somehow we cached an empty response:

https://github.com/webcompat/webcompat.com/blob/master/webcompat/api/endpoints.py#L260-L274

@karlcow should we rip out the caching? I think it's the only place we use that Flask-Cache extension, and it's caused us some problems in the past as well.

[miketaylr]

☑️ My vote is to remove the caching of labels and let the browser use its cache.

[karlcow]

Let me take an issue by chance
https://webcompat.com/issues/583

when we load the issue, we call the URI:
https://webcompat.com/api/issues/583

which is a json file, which has already the labels.

{
  "url": "https://api.github.com/repos/webcompat/web-bugs/issues/583",
  "labels_url": "https://api.github.com/repos/webcompat/web-bugs/issues/583/labels{/name}",
  "comments_url": "https://api.github.com/repos/webcompat/web-bugs/issues/583/comments",
  "events_url": "https://api.github.com/repos/webcompat/web-bugs/issues/583/events",
  "html_url": "https://github.com/webcompat/web-bugs/issues/583",
  "id": 53115789,
  "number": 583,
  "title": "www.torinofc.it - News are updated but not in browser",
  "user": {
    "login": "PeopleInside",
    "id": 5006150,
    "avatar_url": "https://avatars.githubusercontent.com/u/5006150?v=3",
    "gravatar_id": "",
    "url": "https://api.github.com/users/PeopleInside",
    "html_url": "https://github.com/PeopleInside",
    "followers_url": "https://api.github.com/users/PeopleInside/followers",
    "following_url": "https://api.github.com/users/PeopleInside/following{/other_user}",
    "gists_url": "https://api.github.com/users/PeopleInside/gists{/gist_id}",
    "starred_url": "https://api.github.com/users/PeopleInside/starred{/owner}{/repo}",
    "subscriptions_url": "https://api.github.com/users/PeopleInside/subscriptions",
    "organizations_url": "https://api.github.com/users/PeopleInside/orgs",
    "repos_url": "https://api.github.com/users/PeopleInside/repos",
    "events_url": "https://api.github.com/users/PeopleInside/events{/privacy}",
    "received_events_url": "https://api.github.com/users/PeopleInside/received_events",
    "type": "User",
    "site_admin": false
  },
  "labels": [
    {
      "url": "https://api.github.com/repos/webcompat/web-bugs/labels/browser-firefox",
      "name": "browser-firefox",
      "color": "FF4E00"
    },
    {
      "url": "https://api.github.com/repos/webcompat/web-bugs/labels/status-contactready",
      "name": "status-contactready",
      "color": "A1EBBF"
    }
  ],
  "state": "open",
  "locked": false,
  "assignee": null,
  "milestone": null,
  "comments": 3,
  "created_at": "2014-12-30T13:34:39Z",
  "updated_at": "2015-10-20T03:14:31Z",
  "closed_at": null,
  "body": "<!-- @browser: Firefox 34.0 -->\r\n<!-- @ua_header: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 -->\r\n\r\n**URL**: http://www.torinofc.it/\r\n**Browser / Version**: Firefox 34.0\r\n**Operating System**: Windows 8.1\r\n**Problem type**: Don't know but something's wrong.\r\n**Site owner**: No\r\n\r\n**Steps to Reproduce**\r\n1) Navigate to: http://www.torinofc.it/\r\n2) You will see the news of the day\r\n3) Return in this website in next days and news are updated but you will see the old and not the new. In anonimous firefox session you can see correclty in normal, no\r\n\r\nI think is Brwoser issue.\r\nI had installed AdBlock plus on my Firefox but also i have tried to disable this for this website but continue to see the issue.\r\n\r\nSomeone can confirm and find what is the problem?\r\n\r\nThanks\r\n\r\nExpected Behavior:\r\nActual Behavior:\r\n",
  "closed_by": null
}

Why do we need for every issues to call the full list of labels, be cached or not.
https://webcompat.com/api/issues/labels

What I'm hitting at is that if we decide to be in a more controlled environment, there is no need to get the full list of labels, but just the one we allow, and that could be a list managed at the config level. No more caching issues, no more http requests and one less problem without it. 😈 Here @miketaylr should facepalm itself to have clicked the link. Let it sink in your brain 👺

[hallvors]

I'm looking at simplifying some labels-related code..

[karlcow]

Note @hallvors this one is really about the caching of the full labels list. Not the rest ;)

[hallvors]

I'm looking at everything, @karlcow :)

Having a config-level list of labels is basically a cache with manual maintenance. It prevents some things that we may want to allow (like vendors who contribute defining their own browser- labels). I don't think the labels will change too often, and a GitHub PR to change them isn't too much of an overhead, so I'm not against it as such, but we should probably discuss a bit more..

[miketaylr]

Why do we need for every issues to call the full list of labels, be cached or not.

Because we need the label editor to show all the possible labels, not just the labels already associated with an issue.

I guarantee there are better ways to do this. And a config-level list seems like a good way to approach it. We probably need to think through how to deal with the label-tagger-bot and new UA strings (maybe browser-classify-me-and-add-to-the-config is good enough).

I'm gonna go ahead and send a PR removing the caching (because browsers can do their own caching). And we can continue label discussion in a new issue.

[karlcow]

agreed @miketaylr
@hallvors I meant on a specific issue solve the issue, not everything. If there are more things to deal with, open more issues.

[miketaylr]

(a config-list would help us rip out this overly-complicated way to add back namespaces when we don't know the full list in advance:

https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/js/lib/models/issue.js#L197-L210)

[miketaylr]

(env)miket at omg-3 in ~/dev/compat/webcompat.com on issues/773/2*
🐓 pip uninstall Flask-Cache
The directory '/Users/miket/Library/Caches/pip/http' or its parent directory is not owned by the current user and the cache has been disabled. Please check the permissions and owner of that directory. If executing pip with sudo, you may want sudo's -H flag.
Uninstalling Flask-Cache-0.13.1:
  /Users/miket/dev/compat/webcompat.com/env/lib/python2.7/site-packages/Flask_Cache-0.13.1.dist-info/DESCRIPTION.rst
  /Users/miket/dev/compat/webcompat.com/env/lib/python2.7/site-packages/Flask_Cache-0.13.1.dist-info/METADATA
  /Users/miket/dev/compat/webcompat.com/env/lib/python2.7/site-packages/Flask_Cache-0.13.1.dist-info/RECORD
  /Users/miket/dev/compat/webcompat.com/env/lib/python2.7/site-packages/Flask_Cache-0.13.1.dist-info/WHEEL
  /Users/miket/dev/compat/webcompat.com/env/lib/python2.7/site-packages/Flask_Cache-0.13.1.dist-info/metadata.json
  /Users/miket/dev/compat/webcompat.com/env/lib/python2.7/site-packages/Flask_Cache-0.13.1.dist-info/top_level.txt
  /Users/miket/dev/compat/webcompat.com/env/lib/python2.7/site-packages/flask_cache/__init__.py
  /Users/miket/dev/compat/webcompat.com/env/lib/python2.7/site-packages/flask_cache/__init__.pyc
  /Users/miket/dev/compat/webcompat.com/env/lib/python2.7/site-packages/flask_cache/_compat.py
  /Users/miket/dev/compat/webcompat.com/env/lib/python2.7/site-packages/flask_cache/_compat.pyc
  /Users/miket/dev/compat/webcompat.com/env/lib/python2.7/site-packages/flask_cache/backends.py
  /Users/miket/dev/compat/webcompat.com/env/lib/python2.7/site-packages/flask_cache/backends.pyc
  /Users/miket/dev/compat/webcompat.com/env/lib/python2.7/site-packages/flask_cache/jinja2ext.py
  /Users/miket/dev/compat/webcompat.com/env/lib/python2.7/site-packages/flask_cache/jinja2ext.pyc
Proceed (y/n)? y
  Successfully uninstalled Flask-Cache-0.13.1

(env)miket at omg-3 in ~/dev/compat/webcompat.com on issues/773/2*
🐓 python run.py 
 * Running on http://localhost:5000/ (Press CTRL+C to quit)
 * Restarting with stat

[hallvors]

Just for documentation: I got a local instance into a state where it causes the "no element found" error to happen.
What I see is that the backend replies with a 304 status even if the request is not conditional! That looks like a pretty bad bug somewhere in the Flask cache module.

If the request has If-None-Match or If-Modified-Since headers, it is conditional and a 304 response is just fine. However, I pasted the URL into Fiddler's request maker and sent a request with nothing but a UA header - and the backend STILL said 304. That's just silly - it doesn't help a UA who never requested the file before to be told "oh, I've got it cached so I think you should use something cached locally too".

To prove I'm actually seeing this, here's a copy of the HTTP traffic:

GET http://localhost:5000/api/issues/labels HTTP/1.1
User-Agent: Fiddler
Host: localhost:5000

HTTP/1.0 304 NOT MODIFIED
etag: "283dada14a32282803a8ad534d8b388c"
cache-control: private, max-age=60, s-maxage=60
Connection: close
Server: Werkzeug/0.10.4 Python/2.7.6
Date: Thu, 22 Oct 2015 09:07:17 GMT

Obviously, this confuses the hapless browser.
+1 to removing the Flask caching stuff.

[miketaylr]

Aha! Thanks for tracking down the error. I'll do a deploy at some point today and then we'll be down with this particular bug FOREVER~~~~ 🇺🇸 🇫🇷 🎏

[hallvors]

If we were using https://github.com/thadeusb/flask-cache maybe @thadeusb is interested in this thread? Unfortunately I don't know exactly what happened, but is it possible that the Flask backend sent a conditional request to GitHub's API server, and Flask-Cache simply passed on the 304 response to the client?

[hallvors]

(Seems development has stalled on that project though. Oh well.)

[thadeusb]

@hallvors That would be the case. Flask-cache only deals with creating keys and managing objects within an external caching service such as Redis/Memcached. It is not for using etags or browser cache control. For that I would recommend flask-webcache.

[hallvors]

@thadeusb thanks for responding! :)
@miketaylr perhaps the real bug was using Flask-Cache instead of Flask-Webcache..or something? :)

[miketaylr]

Could be! We can consider that in the future. For now let's rely on browsers and GitHub doing the right thing with caching (famous last words...).