Address problems with deprecated AcceptValidHeader.best_match (C4-627) #1588

netsettler · 2022-01-31T06:13:51Z

This PR ports various bits of support from cgap-portal in order to address a problem with continued use of deprecated method AcceptValidHeader.best_match (See C4-627 for details.)

Note that there are unit tests in place that were already passing before this change was made and that check 18114 cases of different parameter combinations to make sure the behavior of these heavily used functions stay stable. I had to make some adjustments because the cgap-portal code exactly as expressed broke 384 of these cases. But because the unit test had good error diagnostics, it was clear what part was causing the problem, and for now I have favored bug-for-bug compatibility and left the behavior in fourfront "as-is" so that the old test now passes correctly. This narrows the differences between cgap-portal and fourfront in the renderers.py file, and enables a useful discussion about which way we should tip things to bring these two files into better alignment. It may in fact be that cgap-portal needs to change, but we can talk about that another day.

For now, the good outcome of this change is that it phases out uses of .best_match (as long as the MIME_TYPE_TRIAGE_MODE variable is left with a value of modern), and that means we won't get a zillion warnings from the unit tests, since this deprecated would be called at least once per unit test for almost all unit tests, resulting in quite a lot of warnings. Not to mention this method really will go away at some point, and it's good to be rid of it.

Fixing this will also help the Python 3.7 effort, though it's good to do even for Python 3.6 because those warnings (described in C4-627) are annoying.

Note, too, that there were some other attempts at a PR to address this that were broader in scope and that didn't get patched for some reason, and grew stale. (PR #1455 and PR #1465). I think those can be ignored for now, though I may go back and try to close them out or make them less stale if this PR, which is narrower, is approved.

…sses what to do in renderers.py, but retain bug-for-bug compatibility with exactly how fourfront was working before. This is done by a relatively-exhaustive check of all the different options in test_renderers to make sure that 18144 previously-working combinations of arguments are honored.

… of warnings. Also, add some unit tests.

netsettler

I've made a few comments to help reviewers get through this. Please feel free to ask if other things are left unclear.

netsettler · 2022-01-31T06:17:45Z

src/encoded/tests/test_renderers.py

@@ -20,8 +25,53 @@ def __init__(self, content_type=None, status_code: int = 200, json=None, content
        super().__init__(status_code=status_code, json=json, content=content, url=url)


+def test_mime_variables():


Note that this and a couple other unit tests are added in this file, but importantly, the function test_should_transform (on original lines 39-136) are unchanged. This gives pretty good confidence that the core functionality being tested here is unaffected in spite of the reimplementation of some functions used therein.

netsettler · 2022-01-31T06:19:05Z

src/encoded/renderers.py

@@ -196,20 +209,6 @@ def security_tween(request):

        return handler(request)

-        # This was commented out when we introduced JWT authentication


This is just getting rid of code that's long been commented out and is not believed to be needed now.

netsettler · 2022-01-31T06:24:41Z

src/encoded/renderers.py

+
+# Note: In cgap-portal, MIME_TYPE_JSON is at the head of this list. In fourfront, MIME_TYPE_HTML is.
+# The cgap-portal behavior might be a bug we should look at bringing into alignment. -kmp 29-Jan-2022
+MIME_TYPES_SUPPORTED = [MIME_TYPE_HTML, MIME_TYPE_JSON, MIME_TYPE_LD_JSON]


This line is intentionally materially different between cgap-portal and fourfront. When I rewrote this code for cgap-portal, as indicated in the comment, I erroneously made the first element of this list be MIME_TYPE_JSON. This wasn't quite right, though is mostly harmless, but it represented a slight incompatible change for cgap-portal that I did not intend. I think the right thing may be to fix cgap-portal in the future, but for now this just highlights the essential difference between the two implementations. A cascade effect of this is that MIME_TYPE_DEFAULT on the next line ends up being MIME_TYPE_HTML here but MIME_TYPE_JSON in cgap-portal. That's probably wrong as well.

This keeps test_should_transform passing unmodified. However, some accommodations to newly ported unit tests in test_renderers.py had to be made to accommodate this shift. I think they're pretty unobjectionable changes, though.

netsettler · 2022-01-31T06:25:46Z

src/encoded/renderers.py

@@ -331,6 +400,22 @@ def should_transform(request, response):
    if response.content_type != 'application/json':
        return False

+    # cgap uses the following extra check that I think is the right thing, but leaving this commented out


This is the other significant difference between cgap-portal and fourfront. See the comment text for details. This makes test_should_transform in test_renderers.py pass properly.

netsettler · 2022-01-31T06:29:03Z

src/encoded/tests/test_renderers.py

+    assert MIME_TYPES_SUPPORTED == [MIME_TYPE_HTML, MIME_TYPE_JSON, MIME_TYPE_LD_JSON]
+    assert MIME_TYPE_DEFAULT == MIME_TYPE_HTML


These two lines in this test are different than in cgap-portal because they have to correspond to differences I had to make in renderers.py to make this a non-breaking change.

netsettler · 2022-01-31T06:29:47Z

src/encoded/tests/test_util.py

+pytestmark = pytest.mark.working
+
+
+# def test_compute_set_difference_one():


This is just opportunistic. These functions are not used any more.

netsettler · 2022-01-31T06:30:06Z

src/encoded/tests/test_util.py

+    assert (t1 - t0).total_seconds() < custom_delay + DELAY_FUZZ_SECONDS
+
+
+# def test_utc_today_str():


We get this function from dcicutils these days so don't need it here.

netsettler · 2022-01-31T06:30:22Z

src/encoded/util.py

-        :raises: exception if types do not match or in compute_set_diferrence_one
-    """
-    return compute_set_difference_one(set(pair), {element})
+# def compute_set_difference_one(s1, s2):


This is unused, as mentioned above.

netsettler · 2022-01-31T06:30:36Z

src/encoded/util.py

@@ -335,8 +335,8 @@ def parameterized_delay_rerun(*args):
 delay_rerun = customized_delay_rerun(sleep_seconds=1)


-def utc_today_str():
-    return datetime.datetime.strftime(datetime.datetime.utcnow(), "%Y-%m-%d")
+# def utc_today_str():


Available from dcicutils.

willronchetti

We should consider adding some dedicated documentation on this file

alexkb0009

Changes in code sections that am familiar-ish with look good to go, didn't check out the unit tests.

netsettler added 4 commits January 28, 2022 04:42

Various cosmetic preparations for the real changes we want to make.

4a7f877

More changes to renderers.

fe05440

Run new renderers support in modern mode, not legacy mode, to get rid…

2cdf587

… of warnings. Also, add some unit tests.

netsettler requested review from alexkb0009 and willronchetti January 31, 2022 06:13

netsettler commented Jan 31, 2022

View reviewed changes

willronchetti approved these changes Jan 31, 2022

View reviewed changes

alexkb0009 approved these changes Jan 31, 2022

View reviewed changes

willronchetti merged commit 6c8a1d8 into master Apr 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address problems with deprecated AcceptValidHeader.best_match (C4-627) #1588

Address problems with deprecated AcceptValidHeader.best_match (C4-627) #1588

netsettler commented Jan 31, 2022 •

edited

Loading

netsettler left a comment

netsettler Jan 31, 2022

netsettler Jan 31, 2022

netsettler Jan 31, 2022

netsettler Jan 31, 2022

netsettler Jan 31, 2022

netsettler Jan 31, 2022

netsettler Jan 31, 2022

netsettler Jan 31, 2022

netsettler Jan 31, 2022

willronchetti left a comment

alexkb0009 left a comment

		@@ -20,8 +25,53 @@ def __init__(self, content_type=None, status_code: int = 200, json=None, content
		super().__init__(status_code=status_code, json=json, content=content, url=url)


		def test_mime_variables():

		@@ -196,20 +209,6 @@ def security_tween(request):

		return handler(request)

		# This was commented out when we introduced JWT authentication

		assert MIME_TYPES_SUPPORTED == [MIME_TYPE_HTML, MIME_TYPE_JSON, MIME_TYPE_LD_JSON]
		assert MIME_TYPE_DEFAULT == MIME_TYPE_HTML

		pytestmark = pytest.mark.working


		# def test_compute_set_difference_one():

		assert (t1 - t0).total_seconds() < custom_delay + DELAY_FUZZ_SECONDS


		# def test_utc_today_str():

Address problems with deprecated AcceptValidHeader.best_match (C4-627) #1588

Address problems with deprecated AcceptValidHeader.best_match (C4-627) #1588

Conversation

netsettler commented Jan 31, 2022 • edited Loading

netsettler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willronchetti left a comment

Choose a reason for hiding this comment

alexkb0009 left a comment

Choose a reason for hiding this comment

netsettler commented Jan 31, 2022 •

edited

Loading