fix the check for whether `is_url` to match all the other ones in codebase #3405

t3chguy · 2018-06-17T10:42:03Z

as this one did not check whether the value was a string and only checked its existence whereas the other two checked for both

partially addresses element-hq/element-web#6250

richvdh

sorry for (a) the epic delay and (b) the large comments-to-diffs ratio

richvdh · 2018-07-27T08:24:28Z

synapse/api/filtering.py

-            is_url = "url" in event.get("content", {})
+
+            content = event.get("content", {})
+            is_url = (


a comment to explain why we are doing this would be good.

richvdh · 2018-07-27T08:25:05Z

synapse/api/filtering.py

+
+            content = event.get("content", {})
+            is_url = (
+                "url" in content


how about isinstance(content.get("url"), basestring) ?

richvdh · 2018-07-27T08:27:01Z

synapse/api/filtering.py

+            content = event.get("content", {})
+            is_url = (
+                "url" in content
+                and isinstance(content["url"], basestring)


now that we're beginning to get things working on python3, it's helpful to avoid python2-only-isms.

in this case, I think you want isinstance(..., six.text_type)

richvdh · 2018-07-27T08:27:54Z

@matrixbot : test this please

richvdh · 2018-07-30T17:00:50Z

ugh sorry @t3chguy could you merge latest develop in?

richvdh · 2018-12-24T09:56:21Z

@t3chguy sorry can you merge develop once more to kick the CI? And comment once it's done so I don't miss it for another 5 months...

…ebase Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

codecov-io · 2018-12-29T23:55:50Z

Codecov Report

Merging #3405 into develop will decrease coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #3405      +/-   ##
===========================================
- Coverage    73.73%   73.73%   -0.01%     
===========================================
  Files          300      300              
  Lines        29751    29753       +2     
  Branches      4878     4878              
===========================================
  Hits         21938    21938              
- Misses        6387     6390       +3     
+ Partials      1426     1425       -1

Impacted Files	Coverage Δ
synapse/api/filtering.py	`97.31% <100%> (+0.03%)`	⬆️
synapse/state/v1.py	`90.69% <0%> (-1.56%)`	⬇️
synapse/handlers/search.py	`80.24% <0%> (ø)`	⬆️
synapse/handlers/federation.py	`61.72% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7843f4...fcd3861. Read the comment docs.

richvdh · 2019-01-04T17:38:35Z

synapse/api/filtering.py

+
+            content = event.get("content", {})
+            # check if there is a string url field in the content for filtering purposes
+            contains_url = isinstance(content.get("url"), text_type)


the spec says "includes only events with a url key in their content"

it doesn't say it has to be a string...

(this might be a good opportunity to clarify the spec so it makes sense)

well, if there are compelling reasons to do so, then sure, let's change the spec. Are there, though?

[if you actually want to check it's a string, you probably want six.string_types rather than six.text_type]

There's not really a use case for a singular-named variable to contain an object/array, and it's somewhat implied it is a string anyways. Might as well just say it is a string.

ok, but (a) that's not quite the same as 'filters have to check for the stringiness' and (b) "might as well" doesn't sound worth the effort of changing the spec to me.

@richvdh, existing code disagrees:

synapse/synapse/storage/events.py

Lines 1575 to 1577 in bc80b3f

contains_url = "url" in content

if contains_url:

contains_url &= isinstance(content["url"], text_type)

richvdh

it seems a shame that we are moving further away from matching the spec. I'd have preferred it if (a) the three (?) copies of this code were factored out to a common instance (b) it actually matched the spec. I don't really understand why we need to check that the url is a unicode.

but suddenly my life is too short to care about this. so, fine, let's ship it and never speak of it again.

Synapse 0.34.1rc1 (2019-01-08) ============================== Features -------- - Special-case a support user for use in verifying behaviour of a given server. The support user does not appear in user directory or monthly active user counts. ([\#4141](#4141), [\#4344](#4344)) - Support for serving .well-known files ([\#4262](#4262)) - Rework SAML2 authentication ([\#4265](#4265), [\#4267](#4267)) - SAML2 authentication: Initialise user display name from SAML2 data ([\#4272](#4272)) - Synapse can now have its conditional/extra dependencies installed by pip. This functionality can be used by using `pip install matrix-synapse[feature]`, where feature is a comma separated list with the possible values `email.enable_notifs`, `matrix-synapse-ldap3`, `postgres`, `resources.consent`, `saml2`, `url_preview`, and `test`. If you want to install all optional dependencies, you can use "all" instead. ([\#4298](#4298), [\#4325](#4325), [\#4327](#4327)) - Add routes for reading account data. ([\#4303](#4303)) - Add opt-in support for v2 rooms ([\#4307](#4307)) - Add a script to generate a clean config file ([\#4315](#4315)) - Return server data in /login response ([\#4319](#4319)) Bugfixes -------- - Fix contains_url check to be consistent with other instances in code-base and check that value is an instance of string. ([\#3405](#3405)) - Fix CAS login when username is not valid in an MXID ([\#4264](#4264)) - Send CORS headers for /media/config ([\#4279](#4279)) - Add 'sandbox' to CSP for media reprository ([\#4284](#4284)) - Make the new landing page prettier. ([\#4294](#4294)) - Fix deleting E2E room keys when using old SQLite versions. ([\#4295](#4295)) - The metric synapse_admin_mau:current previously did not update when config.mau_stats_only was set to True ([\#4305](#4305)) - Fixed per-room account data filters ([\#4309](#4309)) - Fix indentation in default config ([\#4313](#4313)) - Fix synapse:latest docker upload ([\#4316](#4316)) - Fix test_metric.py compatibility with prometheus_client 0.5. Contributed by Maarten de Vries <maarten@de-vri.es>. ([\#4317](#4317)) - Avoid packaging _trial_temp directory in -py3 debian packages ([\#4326](#4326)) - Check jinja version for consent resource ([\#4327](#4327)) - fix NPE in /messages by checking if all events were filtered out ([\#4330](#4330)) - Fix `python -m synapse.config` on Python 3. ([\#4356](#4356)) Deprecations and Removals ------------------------- - Remove the deprecated v1/register API on Python 2. It was never ported to Python 3. ([\#4334](#4334)) Internal Changes ---------------- - Getting URL previews of IP addresses no longer fails on Python 3. ([\#4215](#4215)) - drop undocumented dependency on dateutil ([\#4266](#4266)) - Update the example systemd config to use a virtualenv ([\#4273](#4273)) - Update link to kernel DCO guide ([\#4274](#4274)) - Make isort tox check print diff when it fails ([\#4283](#4283)) - Log room_id in Unknown room errors ([\#4297](#4297)) - Documentation improvements for coturn setup. Contributed by Krithin Sitaram. ([\#4333](#4333)) - Update pull request template to use absolute links ([\#4341](#4341)) - Update README to not lie about required restart when updating TLS certificates ([\#4343](#4343)) - Update debian packaging for compatibility with transitional package ([\#4349](#4349)) - Fix command hint to generate a config file when trying to start without a config file ([\#4353](#4353)) - Add better logging for unexpected errors while sending transactions ([\#4358](#4358))

richvdh reviewed Jul 27, 2018

View reviewed changes

t3chguy force-pushed the t3chguy/fix_contains-url_filtering branch from 05c9dd1 to 9cf2d90 Compare August 1, 2018 21:56

t3chguy force-pushed the t3chguy/fix_contains-url_filtering branch from 25703aa to 580e762 Compare December 29, 2018 23:15

t3chguy added 3 commits December 29, 2018 23:18

fix the check for whether is_url to match all the other ones in cod…

9cd62a2

…ebase Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

make linter happy

f2f76cb

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

apply PR feedback

cfc0e13

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

t3chguy force-pushed the t3chguy/fix_contains-url_filtering branch from 580e762 to cfc0e13 Compare December 29, 2018 23:19

t3chguy added 2 commits December 29, 2018 23:27

comply with isort

41d9ed2

make it even more like the other instance and add newsfragment

fcd3861

richvdh self-requested a review January 2, 2019 07:13

richvdh suggested changes Jan 4, 2019

View reviewed changes

richvdh approved these changes Jan 6, 2019

View reviewed changes

richvdh merged commit a27e501 into matrix-org:develop Jan 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix the check for whether `is_url` to match all the other ones in codebase #3405

fix the check for whether `is_url` to match all the other ones in codebase #3405

t3chguy commented Jun 17, 2018 •

edited by richvdh

Loading

richvdh left a comment

richvdh Jul 27, 2018

richvdh Jul 27, 2018

richvdh Jul 27, 2018

richvdh commented Jul 27, 2018

richvdh commented Jul 30, 2018

richvdh commented Dec 24, 2018

codecov-io commented Dec 29, 2018

richvdh Jan 4, 2019

turt2live Jan 4, 2019

richvdh Jan 4, 2019

turt2live Jan 4, 2019

richvdh Jan 4, 2019

t3chguy Jan 4, 2019

richvdh left a comment

	contains_url = "url" in content
	if contains_url:
	contains_url &= isinstance(content["url"], text_type)

fix the check for whether is_url to match all the other ones in codebase #3405

fix the check for whether is_url to match all the other ones in codebase #3405

Conversation

t3chguy commented Jun 17, 2018 • edited by richvdh Loading

richvdh 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

richvdh commented Jul 27, 2018

richvdh commented Jul 30, 2018

richvdh commented Dec 24, 2018

codecov-io commented Dec 29, 2018

Codecov Report

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

richvdh left a comment

Choose a reason for hiding this comment

fix the check for whether `is_url` to match all the other ones in codebase #3405

fix the check for whether `is_url` to match all the other ones in codebase #3405

t3chguy commented Jun 17, 2018 •

edited by richvdh

Loading