From f41bd1eabe30a19d3f1df9ea7c54fda5eb33ff11 Mon Sep 17 00:00:00 2001 From: Deimos Date: Fri, 31 Jul 2020 12:13:23 -0600 Subject: [PATCH] Upgrade webargs to 6.1.0 This was not a fun upgrade. webargs made some major changes to its approaches in 6.0, which are mostly covered here: https://webargs.readthedocs.io/en/latest/upgrading.html To keep using it on Tildes, this commit had to make the following changes: - Write my own wrapper for use_kwargs that changes some of the default behavior. Specifically, we want the location that data is being loaded from to default to "query" (the query string) instead of webargs' default of "json". We also needed to set the "unknown" behavior on every schema to "exclude" so that the schemas would ignore any data fields they didn't need, since the default behavior is to throw an error, which happens almost everywhere because of Intercooler variables and/or multiple use_kwargs calls for different subsets of the data. - All @pre_load hooks in schemas needed to be rewritten so that they weren't modifying data in-place (copy to a new data dict first). Because webargs is now passing all data through all schemas, modifying in-place could result in an earlier schema modifying data that would then be passed in modified form to the later ones. Specifically, this caused an issue with tags on posting a new topic, where we just wanted to treat the tags as a string, but TopicSchema would convert it to a list in @pre_load. - use_kwargs on every endpoint using non-query data needed to be updated to support the new single-location approach, either replacing an existing locations= with location=, or adding location="form", since form data was no longer used by default. - The code that parsed the errors returned by webargs/Marshmallow ValidationErrors needed to update to handle the additional "level" in the dict of errors, where errors are now split out by location and then field, instead of only by field. - A few other minor updates, like always passing a schema object instead of a class, and never passing a callable (mostly just for simplicity in the wrapper). --- tildes/requirements-dev.txt | 2 +- tildes/requirements.in | 2 +- tildes/requirements.txt | 2 +- tildes/tildes/resources/comment.py | 6 +-- tildes/tildes/resources/group.py | 6 +-- tildes/tildes/resources/message.py | 4 +- tildes/tildes/resources/topic.py | 4 +- tildes/tildes/resources/user.py | 4 +- tildes/tildes/schemas/group.py | 18 ++++++--- tildes/tildes/schemas/listing.py | 20 ++++++---- tildes/tildes/schemas/topic.py | 40 +++++++++++-------- tildes/tildes/schemas/user.py | 37 +++++++++++------ tildes/tildes/views/api/web/comment.py | 18 ++++----- tildes/tildes/views/api/web/group.py | 5 +-- .../tildes/views/api/web/markdown_preview.py | 5 +-- tildes/tildes/views/api/web/message.py | 5 +-- tildes/tildes/views/api/web/topic.py | 15 +++---- tildes/tildes/views/api/web/user.py | 20 +++++----- tildes/tildes/views/bookmarks.py | 4 +- tildes/tildes/views/decorators.py | 30 +++++++++++++- tildes/tildes/views/donate.py | 6 +-- tildes/tildes/views/exceptions.py | 6 ++- tildes/tildes/views/group_wiki_page.py | 8 ++-- tildes/tildes/views/ignored_topics.py | 4 +- tildes/tildes/views/login.py | 12 +++--- tildes/tildes/views/message.py | 6 +-- tildes/tildes/views/notifications.py | 2 +- tildes/tildes/views/register.py | 26 +++++------- tildes/tildes/views/settings.py | 5 ++- tildes/tildes/views/topic.py | 9 ++--- tildes/tildes/views/user.py | 2 +- tildes/tildes/views/votes.py | 4 +- 32 files changed, 195 insertions(+), 142 deletions(-) diff --git a/tildes/requirements-dev.txt b/tildes/requirements-dev.txt index 635086c..9c01b6f 100644 --- a/tildes/requirements-dev.txt +++ b/tildes/requirements-dev.txt @@ -109,7 +109,7 @@ urllib3==1.25.10 # via requests, sentry-sdk venusian==3.0.0 # via cornice, pyramid waitress==1.4.4 # via webtest wcwidth==0.2.5 # via prompt-toolkit -webargs==5.5.3 +webargs==6.1.0 webassets==2.0 # via pyramid-webassets webencodings==0.5.1 # via bleach, html5lib webob==1.8.6 # via pyramid, webtest diff --git a/tildes/requirements.in b/tildes/requirements.in index 9dbd14d..88e2784 100644 --- a/tildes/requirements.in +++ b/tildes/requirements.in @@ -34,6 +34,6 @@ SQLAlchemy SQLAlchemy-Utils stripe titlecase -webargs==5.5.3 +webargs wrapt zope.sqlalchemy diff --git a/tildes/requirements.txt b/tildes/requirements.txt index ea75034..f7fa916 100644 --- a/tildes/requirements.txt +++ b/tildes/requirements.txt @@ -67,7 +67,7 @@ translationstring==1.4 # via pyramid urllib3==1.25.10 # via requests, sentry-sdk venusian==3.0.0 # via cornice, pyramid wcwidth==0.2.5 # via prompt-toolkit -webargs==5.5.3 +webargs==6.1.0 webassets==2.0 # via pyramid-webassets webencodings==0.5.1 # via bleach, html5lib webob==1.8.6 # via pyramid diff --git a/tildes/tildes/resources/comment.py b/tildes/tildes/resources/comment.py index a89fccc..45aefa6 100644 --- a/tildes/tildes/resources/comment.py +++ b/tildes/tildes/resources/comment.py @@ -5,15 +5,15 @@ from pyramid.httpexceptions import HTTPForbidden, HTTPNotFound from pyramid.request import Request -from webargs.pyramidparser import use_kwargs from tildes.lib.id import id36_to_id from tildes.models.comment import Comment, CommentNotification from tildes.resources import get_resource from tildes.schemas.comment import CommentSchema +from tildes.views.decorators import use_kwargs -@use_kwargs(CommentSchema(only=("comment_id36",)), locations=("matchdict",)) +@use_kwargs(CommentSchema(only=("comment_id36",)), location="matchdict") def comment_by_id36(request: Request, comment_id36: str) -> Comment: """Get a comment specified by {comment_id36} in the route (or 404).""" query = ( @@ -28,7 +28,7 @@ def comment_by_id36(request: Request, comment_id36: str) -> Comment: raise HTTPNotFound("Comment not found (or it was deleted)") -@use_kwargs(CommentSchema(only=("comment_id36",)), locations=("matchdict",)) +@use_kwargs(CommentSchema(only=("comment_id36",)), location="matchdict") def notification_by_comment_id36( request: Request, comment_id36: str ) -> CommentNotification: diff --git a/tildes/tildes/resources/group.py b/tildes/tildes/resources/group.py index 13b2698..b95f964 100644 --- a/tildes/tildes/resources/group.py +++ b/tildes/tildes/resources/group.py @@ -7,16 +7,16 @@ from marshmallow.fields import String from pyramid.httpexceptions import HTTPMovedPermanently, HTTPNotFound from pyramid.request import Request from sqlalchemy_utils import Ltree -from webargs.pyramidparser import use_kwargs from tildes.models.group import Group, GroupWikiPage from tildes.resources import get_resource from tildes.schemas.group import GroupSchema +from tildes.views.decorators import use_kwargs @use_kwargs( GroupSchema(only=("path",), context={"fix_path_capitalization": True}), - locations=("matchdict",), + location="matchdict", ) def group_by_path(request: Request, path: str) -> Group: """Get a group specified by {path} in the route (or 404).""" @@ -35,7 +35,7 @@ def group_by_path(request: Request, path: str) -> Group: return get_resource(request, query) -@use_kwargs({"wiki_page_path": String()}, locations=("matchdict",)) +@use_kwargs({"wiki_page_path": String()}, location="matchdict") def group_wiki_page_by_path(request: Request, wiki_page_path: str) -> GroupWikiPage: """Get a group's wiki page by its path (or 404).""" group = group_by_path(request) # pylint: disable=no-value-for-parameter diff --git a/tildes/tildes/resources/message.py b/tildes/tildes/resources/message.py index 25c30db..a4ff203 100644 --- a/tildes/tildes/resources/message.py +++ b/tildes/tildes/resources/message.py @@ -4,16 +4,16 @@ """Root factories for messages.""" from pyramid.request import Request -from webargs.pyramidparser import use_kwargs from tildes.lib.id import id36_to_id from tildes.models.message import MessageConversation from tildes.resources import get_resource from tildes.schemas.message import MessageConversationSchema +from tildes.views.decorators import use_kwargs @use_kwargs( - MessageConversationSchema(only=("conversation_id36",)), locations=("matchdict",) + MessageConversationSchema(only=("conversation_id36",)), location="matchdict" ) def message_conversation_by_id36( request: Request, conversation_id36: str diff --git a/tildes/tildes/resources/topic.py b/tildes/tildes/resources/topic.py index 541ba10..e5e7ac7 100644 --- a/tildes/tildes/resources/topic.py +++ b/tildes/tildes/resources/topic.py @@ -5,15 +5,15 @@ from pyramid.httpexceptions import HTTPFound, HTTPNotFound from pyramid.request import Request -from webargs.pyramidparser import use_kwargs from tildes.lib.id import id36_to_id from tildes.models.topic import Topic from tildes.resources import get_resource from tildes.schemas.topic import TopicSchema +from tildes.views.decorators import use_kwargs -@use_kwargs(TopicSchema(only=("topic_id36",)), locations=("matchdict",)) +@use_kwargs(TopicSchema(only=("topic_id36",)), location="matchdict") def topic_by_id36(request: Request, topic_id36: str) -> Topic: """Get a topic specified by {topic_id36} in the route (or 404).""" try: diff --git a/tildes/tildes/resources/user.py b/tildes/tildes/resources/user.py index 79b65ab..f80b735 100644 --- a/tildes/tildes/resources/user.py +++ b/tildes/tildes/resources/user.py @@ -4,14 +4,14 @@ """Root factories for users.""" from pyramid.request import Request -from webargs.pyramidparser import use_kwargs from tildes.models.user import User from tildes.resources import get_resource from tildes.schemas.user import UserSchema +from tildes.views.decorators import use_kwargs -@use_kwargs(UserSchema(only=("username",)), locations=("matchdict",)) +@use_kwargs(UserSchema(only=("username",)), location="matchdict") def user_by_username(request: Request, username: str) -> User: """Get a user specified by {username} in the route or 404 if not found.""" query = request.query(User).include_deleted().filter(User.username == username) diff --git a/tildes/tildes/schemas/group.py b/tildes/tildes/schemas/group.py index 2b51824..1659cb8 100644 --- a/tildes/tildes/schemas/group.py +++ b/tildes/tildes/schemas/group.py @@ -47,10 +47,14 @@ class GroupSchema(Schema): if not self.context.get("fix_path_capitalization"): return data - if "path" in data and isinstance(data["path"], str): - data["path"] = data["path"].lower() + if "path" not in data or not isinstance(data["path"], str): + return data + + new_data = data.copy() - return data + new_data["path"] = new_data["path"].lower() + + return new_data @validates("path") def validate_path(self, value: sqlalchemy_utils.Ltree) -> None: @@ -71,11 +75,13 @@ class GroupSchema(Schema): if "sidebar_markdown" not in data: return data + new_data = data.copy() + # if the value is empty, convert it to None - if not data["sidebar_markdown"] or data["sidebar_markdown"].isspace(): - data["sidebar_markdown"] = None + if not new_data["sidebar_markdown"] or new_data["sidebar_markdown"].isspace(): + new_data["sidebar_markdown"] = None - return data + return new_data def is_valid_group_path(path: str) -> bool: diff --git a/tildes/tildes/schemas/listing.py b/tildes/tildes/schemas/listing.py index 5a797e0..c11508f 100644 --- a/tildes/tildes/schemas/listing.py +++ b/tildes/tildes/schemas/listing.py @@ -46,10 +46,12 @@ class TopicListingSchema(PaginatedListingSchema): if "rank_start" not in self.fields: return data - if not (data.get("before") or data.get("after")): - data["n"] = 1 + new_data = data.copy() - return data + if not (new_data.get("before") or new_data.get("after")): + new_data["n"] = 1 + + return new_data class MixedListingSchema(PaginatedListingSchema): @@ -71,10 +73,12 @@ class MixedListingSchema(PaginatedListingSchema): to the topic with ID36 "123". "c-123" also works, for comments. """ # pylint: disable=unused-argument + new_data = data.copy() + keys = ("after", "before") for key in keys: - value = data.get(key) + value = new_data.get(key) if not value: continue @@ -83,12 +87,12 @@ class MixedListingSchema(PaginatedListingSchema): continue if type_char == "c": - data["anchor_type"] = "comment" + new_data["anchor_type"] = "comment" elif type_char == "t": - data["anchor_type"] = "topic" + new_data["anchor_type"] = "topic" else: continue - data[key] = id36 + new_data[key] = id36 - return data + return new_data diff --git a/tildes/tildes/schemas/topic.py b/tildes/tildes/schemas/topic.py index c52aa2c..dc76272 100644 --- a/tildes/tildes/schemas/topic.py +++ b/tildes/tildes/schemas/topic.py @@ -43,10 +43,12 @@ class TopicSchema(Schema): if "title" not in data: return data + new_data = data.copy() + # strip any trailing periods - data["title"] = data["title"].rstrip(".") + new_data["title"] = new_data["title"].rstrip(".") - return data + return new_data @pre_load def prepare_tags(self, data: dict, many: bool, partial: Any) -> dict: @@ -55,9 +57,11 @@ class TopicSchema(Schema): if "tags" not in data: return data + new_data = data.copy() + tags: typing.List[str] = [] - for tag in data["tags"]: + for tag in new_data["tags"]: tag = tag.lower() # replace underscores with spaces @@ -84,9 +88,9 @@ class TopicSchema(Schema): tags.append(tag) - data["tags"] = tags + new_data["tags"] = tags - return data + return new_data @validates("tags") def validate_tags(self, value: typing.List[str]) -> None: @@ -112,11 +116,13 @@ class TopicSchema(Schema): if "markdown" not in data: return data + new_data = data.copy() + # if the value is empty, convert it to None - if not data["markdown"] or data["markdown"].isspace(): - data["markdown"] = None + if not new_data["markdown"] or new_data["markdown"].isspace(): + new_data["markdown"] = None - return data + return new_data @pre_load def prepare_link(self, data: dict, many: bool, partial: Any) -> dict: @@ -125,23 +131,25 @@ class TopicSchema(Schema): if "link" not in data: return data + new_data = data.copy() + # remove leading/trailing whitespace - data["link"] = data["link"].strip() + new_data["link"] = new_data["link"].strip() # if the value is empty, convert it to None - if not data["link"]: - data["link"] = None - return data + if not new_data["link"]: + new_data["link"] = None + return new_data # prepend http:// to the link if it doesn't have a scheme - parsed = urlparse(data["link"]) + parsed = urlparse(new_data["link"]) if not parsed.scheme: - data["link"] = "http://" + data["link"] + new_data["link"] = "http://" + new_data["link"] # run the link through the url-transformation process - data["link"] = apply_url_transformations(data["link"]) + new_data["link"] = apply_url_transformations(new_data["link"]) - return data + return new_data @validates_schema def link_or_markdown(self, data: dict, many: bool, partial: Any) -> None: diff --git a/tildes/tildes/schemas/user.py b/tildes/tildes/schemas/user.py index ffe0686..44bf55f 100644 --- a/tildes/tildes/schemas/user.py +++ b/tildes/tildes/schemas/user.py @@ -63,10 +63,17 @@ class UserSchema(Schema): def anonymize_username(self, data: dict, many: bool) -> dict: """Hide the username if the dumping context specifies to do so.""" # pylint: disable=unused-argument - if "username" in data and self.context.get("hide_username"): - data["username"] = "" + if not self.context.get("hide_username"): + return data + + if "username" not in data: + return data + + new_data = data.copy() - return data + new_data["username"] = "" + + return new_data @validates_schema def username_pass_not_substrings( @@ -116,9 +123,11 @@ class UserSchema(Schema): if "username" not in data: return data - data["username"] = data["username"].strip() + new_data = data.copy() + + new_data["username"] = new_data["username"].strip() - return data + return new_data @pre_load def prepare_email_address(self, data: dict, many: bool, partial: Any) -> dict: @@ -127,14 +136,16 @@ class UserSchema(Schema): if "email_address" not in data: return data + new_data = data.copy() + # remove any leading/trailing whitespace - data["email_address"] = data["email_address"].strip() + new_data["email_address"] = new_data["email_address"].strip() # if the value is empty, convert it to None - if not data["email_address"] or data["email_address"].isspace(): - data["email_address"] = None + if not new_data["email_address"] or new_data["email_address"].isspace(): + new_data["email_address"] = None - return data + return new_data @pre_load def prepare_bio_markdown(self, data: dict, many: bool, partial: Any) -> dict: @@ -143,11 +154,13 @@ class UserSchema(Schema): if "bio_markdown" not in data: return data + new_data = data.copy() + # if the value is empty, convert it to None - if not data["bio_markdown"] or data["bio_markdown"].isspace(): - data["bio_markdown"] = None + if not new_data["bio_markdown"] or new_data["bio_markdown"].isspace(): + new_data["bio_markdown"] = None - return data + return new_data def is_valid_username(username: str) -> bool: diff --git a/tildes/tildes/views/api/web/comment.py b/tildes/tildes/views/api/web/comment.py index 7b4dead..f5b1e74 100644 --- a/tildes/tildes/views/api/web/comment.py +++ b/tildes/tildes/views/api/web/comment.py @@ -9,7 +9,6 @@ from pyramid.request import Request from pyramid.response import Response from sqlalchemy.exc import IntegrityError from sqlalchemy.orm.exc import FlushError -from webargs.pyramidparser import use_kwargs from tildes.enums import CommentLabelOption, CommentNotificationType, LogEventType from tildes.models.comment import ( @@ -22,7 +21,7 @@ from tildes.models.comment import ( from tildes.models.log import LogComment from tildes.schemas.comment import CommentLabelSchema, CommentSchema from tildes.views import IC_NOOP -from tildes.views.decorators import ic_view_config, rate_limit_view +from tildes.views.decorators import ic_view_config, rate_limit_view, use_kwargs def _mark_comment_read_from_interaction(request: Request, comment: Comment) -> None: @@ -47,7 +46,7 @@ def _mark_comment_read_from_interaction(request: Request, comment: Comment) -> N renderer="single_comment.jinja2", permission="comment", ) -@use_kwargs(CommentSchema(only=("markdown",))) +@use_kwargs(CommentSchema(only=("markdown",)), location="form") @rate_limit_view("comment_post") def post_toplevel_comment(request: Request, markdown: str) -> dict: """Post a new top-level comment on a topic with Intercooler.""" @@ -83,7 +82,7 @@ def post_toplevel_comment(request: Request, markdown: str) -> dict: renderer="single_comment.jinja2", permission="reply", ) -@use_kwargs(CommentSchema(only=("markdown",))) +@use_kwargs(CommentSchema(only=("markdown",)), location="form") @rate_limit_view("comment_post") def post_comment_reply(request: Request, markdown: str) -> dict: """Post a reply to a comment with Intercooler.""" @@ -159,7 +158,7 @@ def get_comment_edit(request: Request) -> dict: renderer="comment_contents.jinja2", permission="edit", ) -@use_kwargs(CommentSchema(only=("markdown",))) +@use_kwargs(CommentSchema(only=("markdown",)), location="form") def patch_comment(request: Request, markdown: str) -> dict: """Update a comment with Intercooler.""" comment = request.context @@ -260,9 +259,8 @@ def delete_vote_comment(request: Request) -> dict: permission="label", renderer="comment_contents.jinja2", ) -@use_kwargs(CommentLabelSchema(only=("name",)), locations=("matchdict",)) -# need to specify only "form" location for reason, or it will crash by looking for JSON -@use_kwargs(CommentLabelSchema(only=("reason",)), locations=("form",)) +@use_kwargs(CommentLabelSchema(only=("name",)), location="matchdict") +@use_kwargs(CommentLabelSchema(only=("reason",)), location="form") def put_label_comment( request: Request, name: CommentLabelOption, reason: str ) -> Response: @@ -308,7 +306,7 @@ def put_label_comment( permission="label", renderer="comment_contents.jinja2", ) -@use_kwargs(CommentLabelSchema(only=("name",)), locations=("matchdict",)) +@use_kwargs(CommentLabelSchema(only=("name",)), location="matchdict") def delete_label_comment(request: Request, name: CommentLabelOption) -> Response: """Remove a label (that the user previously added) from a comment.""" comment = request.context @@ -337,7 +335,7 @@ def delete_label_comment(request: Request, name: CommentLabelOption) -> Response @ic_view_config( route_name="comment_mark_read", request_method="PUT", permission="mark_read" ) -@use_kwargs({"mark_all_previous": Boolean(missing=False)}, locations=("query",)) +@use_kwargs({"mark_all_previous": Boolean(missing=False)}, location="query") def put_mark_comments_read(request: Request, mark_all_previous: bool) -> Response: """Mark comment(s) read, clearing notifications. diff --git a/tildes/tildes/views/api/web/group.py b/tildes/tildes/views/api/web/group.py index 20f16c4..ccb90ab 100644 --- a/tildes/tildes/views/api/web/group.py +++ b/tildes/tildes/views/api/web/group.py @@ -8,7 +8,6 @@ from typing import Optional from pyramid.request import Request from sqlalchemy.dialects.postgresql import insert from sqlalchemy.exc import IntegrityError -from webargs.pyramidparser import use_kwargs from zope.sqlalchemy import mark_changed from tildes.enums import TopicSortOption @@ -17,7 +16,7 @@ from tildes.models.group import Group, GroupSubscription from tildes.models.user import UserGroupSettings from tildes.schemas.fields import Enum, ShortTimePeriod from tildes.views import IC_NOOP -from tildes.views.decorators import ic_view_config +from tildes.views.decorators import ic_view_config, use_kwargs @ic_view_config( @@ -89,7 +88,7 @@ def delete_subscribe_group(request: Request) -> dict: "order": Enum(TopicSortOption), "period": ShortTimePeriod(allow_none=True, missing=None), }, - locations=("form",), # will crash due to trying to find JSON data without this + location="form", ) def patch_group_user_settings( request: Request, order: TopicSortOption, period: Optional[SimpleHoursPeriod] diff --git a/tildes/tildes/views/api/web/markdown_preview.py b/tildes/tildes/views/api/web/markdown_preview.py index 52b15f5..66b7f49 100644 --- a/tildes/tildes/views/api/web/markdown_preview.py +++ b/tildes/tildes/views/api/web/markdown_preview.py @@ -4,11 +4,10 @@ """Web API endpoint for previewing Markdown.""" from pyramid.request import Request -from webargs.pyramidparser import use_kwargs from tildes.lib.markdown import convert_markdown_to_safe_html from tildes.schemas.group_wiki_page import GroupWikiPageSchema -from tildes.views.decorators import ic_view_config +from tildes.views.decorators import ic_view_config, use_kwargs @ic_view_config( @@ -17,7 +16,7 @@ from tildes.views.decorators import ic_view_config renderer="markdown_preview.jinja2", ) # uses GroupWikiPageSchema because it should always have the highest max_length -@use_kwargs(GroupWikiPageSchema(only=("markdown",))) +@use_kwargs(GroupWikiPageSchema(only=("markdown",)), location="form") def markdown_preview(request: Request, markdown: str) -> dict: """Render the provided text as Markdown.""" # pylint: disable=unused-argument diff --git a/tildes/tildes/views/api/web/message.py b/tildes/tildes/views/api/web/message.py index 82e1d52..8a324b9 100644 --- a/tildes/tildes/views/api/web/message.py +++ b/tildes/tildes/views/api/web/message.py @@ -4,11 +4,10 @@ """Web API endpoints related to messages.""" from pyramid.request import Request -from webargs.pyramidparser import use_kwargs from tildes.models.message import MessageReply from tildes.schemas.message import MessageReplySchema -from tildes.views.decorators import ic_view_config +from tildes.views.decorators import ic_view_config, use_kwargs @ic_view_config( @@ -17,7 +16,7 @@ from tildes.views.decorators import ic_view_config renderer="single_message.jinja2", permission="reply", ) -@use_kwargs(MessageReplySchema(only=("markdown",))) +@use_kwargs(MessageReplySchema(only=("markdown",)), location="form") def post_message_reply(request: Request, markdown: str) -> dict: """Post a reply to a message conversation with Intercooler.""" conversation = request.context diff --git a/tildes/tildes/views/api/web/topic.py b/tildes/tildes/views/api/web/topic.py index 64f4138..2269c62 100644 --- a/tildes/tildes/views/api/web/topic.py +++ b/tildes/tildes/views/api/web/topic.py @@ -9,7 +9,6 @@ from pyramid.httpexceptions import HTTPNotFound from pyramid.request import Request from pyramid.response import Response from sqlalchemy.exc import IntegrityError -from webargs.pyramidparser import use_kwargs from tildes.enums import LogEventType from tildes.models.group import Group @@ -18,7 +17,7 @@ from tildes.models.topic import Topic, TopicBookmark, TopicIgnore, TopicVote from tildes.schemas.group import GroupSchema from tildes.schemas.topic import TopicSchema from tildes.views import IC_NOOP -from tildes.views.decorators import ic_view_config +from tildes.views.decorators import ic_view_config, use_kwargs @ic_view_config( @@ -50,7 +49,7 @@ def get_topic_contents(request: Request) -> dict: renderer="topic_contents.jinja2", permission="edit", ) -@use_kwargs(TopicSchema(only=("markdown",))) +@use_kwargs(TopicSchema(only=("markdown",)), location="form") def patch_topic(request: Request, markdown: str) -> dict: """Update a topic with Intercooler.""" topic = request.context @@ -158,7 +157,9 @@ def get_topic_tags(request: Request) -> dict: renderer="topic_tags.jinja2", permission="tag", ) -@use_kwargs({"tags": String(missing=""), "conflict_check": String(missing="")}) +@use_kwargs( + {"tags": String(missing=""), "conflict_check": String(missing="")}, location="form" +) def put_tag_topic(request: Request, tags: str, conflict_check: str) -> dict: """Apply tags to a topic with Intercooler.""" topic = request.context @@ -225,7 +226,7 @@ def get_topic_group(request: Request) -> dict: request_method="PATCH", permission="move", ) -@use_kwargs(GroupSchema(only=("path",))) +@use_kwargs(GroupSchema(only=("path",)), location="form") def patch_move_topic(request: Request, path: str) -> dict: """Move a topic to a different group with Intercooler.""" topic = request.context @@ -334,7 +335,7 @@ def get_topic_title(request: Request) -> dict: request_method="PATCH", permission="edit_title", ) -@use_kwargs(TopicSchema(only=("title",))) +@use_kwargs(TopicSchema(only=("title",)), location="form") def patch_topic_title(request: Request, title: str) -> dict: """Edit a topic's title with Intercooler.""" topic = request.context @@ -373,7 +374,7 @@ def get_topic_link(request: Request) -> dict: request_method="PATCH", permission="edit_link", ) -@use_kwargs(TopicSchema(only=("link",))) +@use_kwargs(TopicSchema(only=("link",)), location="form") def patch_topic_link(request: Request, link: str) -> dict: """Edit a topic's link with Intercooler.""" topic = request.context diff --git a/tildes/tildes/views/api/web/user.py b/tildes/tildes/views/api/web/user.py index 536127c..a0bc23c 100644 --- a/tildes/tildes/views/api/web/user.py +++ b/tildes/tildes/views/api/web/user.py @@ -17,7 +17,6 @@ from pyramid.httpexceptions import ( from pyramid.request import Request from pyramid.response import Response from sqlalchemy.exc import IntegrityError -from webargs.pyramidparser import use_kwargs from tildes.enums import LogEventType, TopicSortOption from tildes.lib.datetime import SimpleHoursPeriod @@ -28,7 +27,7 @@ from tildes.schemas.fields import Enum, ShortTimePeriod from tildes.schemas.topic import TopicSchema from tildes.schemas.user import UserSchema from tildes.views import IC_NOOP -from tildes.views.decorators import ic_view_config +from tildes.views.decorators import ic_view_config, use_kwargs PASSWORD_FIELD = UserSchema(only=("password",)).fields["password"] @@ -45,7 +44,8 @@ PASSWORD_FIELD = UserSchema(only=("password",)).fields["password"] "old_password": PASSWORD_FIELD, "new_password": PASSWORD_FIELD, "new_password_confirm": PASSWORD_FIELD, - } + }, + location="form", ) def patch_change_password( request: Request, old_password: str, new_password: str, new_password_confirm: str @@ -70,7 +70,7 @@ def patch_change_password( request_param="ic-trigger-name=account-recovery-email", permission="change_settings", ) -@use_kwargs(UserSchema(only=("email_address", "email_address_note"))) +@use_kwargs(UserSchema(only=("email_address", "email_address_note")), location="form") def patch_change_email_address( request: Request, email_address: str, email_address_note: str ) -> Response: @@ -102,7 +102,7 @@ def patch_change_email_address( renderer="two_factor_enabled.jinja2", permission="change_settings", ) -@use_kwargs({"code": String()}) +@use_kwargs({"code": String()}, location="form") def post_enable_two_factor(request: Request, code: str) -> dict: """Enable two-factor authentication for the user.""" user = request.context @@ -132,7 +132,7 @@ def post_enable_two_factor(request: Request, code: str) -> dict: renderer="two_factor_disabled.jinja2", permission="change_settings", ) -@use_kwargs({"code": String()}) +@use_kwargs({"code": String()}, location="form") def post_disable_two_factor(request: Request, code: str) -> Response: """Disable two-factor authentication for the user.""" if not request.user.is_correct_two_factor_code(code): @@ -152,7 +152,7 @@ def post_disable_two_factor(request: Request, code: str) -> Response: renderer="two_factor_backup_codes.jinja2", permission="change_settings", ) -@use_kwargs({"code": String()}) +@use_kwargs({"code": String()}, location="form") def post_view_two_factor_backup_codes(request: Request, code: str) -> Response: """Show the user their two-factor authentication backup codes.""" user = request.context @@ -294,7 +294,7 @@ def patch_change_account_default_theme(request: Request) -> Response: request_param="ic-trigger-name=user-bio", permission="change_settings", ) -@use_kwargs({"markdown": String()}) +@use_kwargs({"markdown": String()}, location="form") def patch_change_user_bio(request: Request, markdown: str) -> dict: """Update a user's bio.""" user = request.context @@ -353,7 +353,7 @@ def get_invite_code(request: Request) -> dict: "order": Enum(TopicSortOption), "period": ShortTimePeriod(allow_none=True, missing=None), }, - locations=("form",), # will crash due to trying to find JSON data without this + location="form", ) def put_default_listing_options( request: Request, order: TopicSortOption, period: Optional[SimpleHoursPeriod] @@ -375,7 +375,7 @@ def put_default_listing_options( request_method="PUT", permission="change_settings", ) -@use_kwargs({"tags": String()}) +@use_kwargs({"tags": String()}, location="form") def put_filtered_topic_tags(request: Request, tags: str) -> dict: """Update a user's filtered topic tags list.""" if not tags or tags.isspace(): diff --git a/tildes/tildes/views/bookmarks.py b/tildes/tildes/views/bookmarks.py index 64d81be..728cfe0 100644 --- a/tildes/tildes/views/bookmarks.py +++ b/tildes/tildes/views/bookmarks.py @@ -5,16 +5,16 @@ from typing import Optional, Type, Union from pyramid.request import Request from pyramid.view import view_config from sqlalchemy.sql import desc -from webargs.pyramidparser import use_kwargs from tildes.models.comment import Comment, CommentBookmark from tildes.models.topic import Topic, TopicBookmark from tildes.schemas.fields import PostType from tildes.schemas.listing import PaginatedListingSchema +from tildes.views.decorators import use_kwargs @view_config(route_name="bookmarks", renderer="bookmarks.jinja2") -@use_kwargs(PaginatedListingSchema) +@use_kwargs(PaginatedListingSchema()) @use_kwargs({"post_type": PostType(data_key="type", missing="topic")}) def get_bookmarks( request: Request, diff --git a/tildes/tildes/views/decorators.py b/tildes/tildes/views/decorators.py index cacbef0..a697e71 100644 --- a/tildes/tildes/views/decorators.py +++ b/tildes/tildes/views/decorators.py @@ -3,11 +3,39 @@ """Contains decorators for view functions.""" -from typing import Any, Callable +from typing import Any, Callable, Dict, Union +from marshmallow import EXCLUDE +from marshmallow.fields import Field +from marshmallow.schema import Schema from pyramid.httpexceptions import HTTPFound from pyramid.request import Request from pyramid.view import view_config +from webargs import dict2schema, pyramidparser + + +def use_kwargs( + argmap: Union[Schema, Dict[str, Field]], location: str = "query", **kwargs: Any +) -> Callable: + """Wrap the webargs @use_kwargs decorator with preferred default modifications. + + Primarily, we want the location argument to default to "query" so that the data + comes from the query string. As of version 6.0, webargs defaults to "json", which is + almost never correct for Tildes. + + We also need to set every schema's behavior for unknown fields to "exclude", so that + it just ignores them, instead of erroring when there's unexpected data (as there + almost always is, especially because of Intercooler). + """ + # convert a dict argmap to a Schema (the same way webargs would on its own) + if isinstance(argmap, dict): + argmap = dict2schema(argmap)() + + assert isinstance(argmap, Schema) # tell mypy the type is more restricted now + + argmap.unknown = EXCLUDE + + return pyramidparser.use_kwargs(argmap, location=location, **kwargs) def ic_view_config(**kwargs: Any) -> Callable: diff --git a/tildes/tildes/views/donate.py b/tildes/tildes/views/donate.py index 5a4c897..a0dbfcd 100644 --- a/tildes/tildes/views/donate.py +++ b/tildes/tildes/views/donate.py @@ -10,10 +10,9 @@ from pyramid.httpexceptions import HTTPInternalServerError from pyramid.request import Request from pyramid.security import NO_PERMISSION_REQUIRED from pyramid.view import view_config -from webargs.pyramidparser import use_kwargs from tildes.metrics import incr_counter -from tildes.views.decorators import rate_limit_view +from tildes.views.decorators import rate_limit_view, use_kwargs @view_config( @@ -39,7 +38,8 @@ def get_donate_stripe(request: Request) -> dict: "amount": Float(required=True, validate=Range(min=1.0)), "currency": String(required=True, validate=OneOf(("CAD", "USD"))), "interval": String(required=True, validate=OneOf(("onetime", "month", "year"))), - } + }, + location="form", ) @rate_limit_view("donate_stripe") def post_donate_stripe( diff --git a/tildes/tildes/views/exceptions.py b/tildes/tildes/views/exceptions.py index 9a4ca06..2ca7045 100644 --- a/tildes/tildes/views/exceptions.py +++ b/tildes/tildes/views/exceptions.py @@ -28,7 +28,11 @@ from tildes.models.group import Group def errors_from_validationerror(validation_error: ValidationError) -> Sequence[str]: """Extract errors from a marshmallow ValidationError into a displayable format.""" - errors_by_field = validation_error.normalized_messages() + # as of webargs 6.0, errors are inside a nested dict, where the first level should + # always be a single-item dict with the key representing the "location" of the data + # (e.g. query, form, etc.) - we don't care about that, so just skip that level + errors_by_location = validation_error.normalized_messages() + errors_by_field = list(errors_by_location.values())[0] error_strings = [] for field, errors in errors_by_field.items(): diff --git a/tildes/tildes/views/group_wiki_page.py b/tildes/tildes/views/group_wiki_page.py index 35937f8..31ed8f2 100644 --- a/tildes/tildes/views/group_wiki_page.py +++ b/tildes/tildes/views/group_wiki_page.py @@ -6,11 +6,11 @@ from pyramid.httpexceptions import HTTPFound from pyramid.request import Request from pyramid.view import view_config -from webargs.pyramidparser import use_kwargs from tildes.models.group import GroupWikiPage from tildes.schemas.fields import SimpleString from tildes.schemas.group_wiki_page import GroupWikiPageSchema +from tildes.views.decorators import use_kwargs @view_config(route_name="group_wiki", renderer="group_wiki.jinja2") @@ -65,7 +65,7 @@ def get_wiki_new_page_form(request: Request) -> dict: @view_config( route_name="group_wiki", request_method="POST", permission="wiki_page_create" ) -@use_kwargs(GroupWikiPageSchema()) +@use_kwargs(GroupWikiPageSchema(), location="form") def post_group_wiki(request: Request, page_name: str, markdown: str) -> HTTPFound: """Create a new wiki page in a group.""" group = request.context @@ -94,8 +94,8 @@ def get_wiki_edit_page_form(request: Request) -> dict: @view_config(route_name="group_wiki_page", request_method="POST", permission="edit") -@use_kwargs(GroupWikiPageSchema(only=("markdown",))) -@use_kwargs({"edit_message": SimpleString(max_length=80)}) +@use_kwargs(GroupWikiPageSchema(only=("markdown",)), location="form") +@use_kwargs({"edit_message": SimpleString(max_length=80)}, location="form") def post_group_wiki_page(request: Request, markdown: str, edit_message: str) -> dict: """Apply an edit to a single group wiki page.""" page = request.context diff --git a/tildes/tildes/views/ignored_topics.py b/tildes/tildes/views/ignored_topics.py index 372116b..d79025f 100644 --- a/tildes/tildes/views/ignored_topics.py +++ b/tildes/tildes/views/ignored_topics.py @@ -5,14 +5,14 @@ from typing import Optional from pyramid.request import Request from pyramid.view import view_config from sqlalchemy.sql import desc -from webargs.pyramidparser import use_kwargs from tildes.models.topic import Topic, TopicIgnore from tildes.schemas.listing import PaginatedListingSchema +from tildes.views.decorators import use_kwargs @view_config(route_name="ignored_topics", renderer="ignored_topics.jinja2") -@use_kwargs(PaginatedListingSchema) +@use_kwargs(PaginatedListingSchema()) def get_ignored_topics( request: Request, after: Optional[str], before: Optional[str], per_page: int, ) -> dict: diff --git a/tildes/tildes/views/login.py b/tildes/tildes/views/login.py index d37cd32..b6f519d 100644 --- a/tildes/tildes/views/login.py +++ b/tildes/tildes/views/login.py @@ -14,14 +14,13 @@ from pyramid.request import Request from pyramid.response import Response from pyramid.security import NO_PERMISSION_REQUIRED, remember from pyramid.view import view_config -from webargs.pyramidparser import use_kwargs from tildes.enums import LogEventType from tildes.metrics import incr_counter from tildes.models.log import Log from tildes.models.user import User from tildes.schemas.user import UserSchema -from tildes.views.decorators import not_logged_in, rate_limit_view +from tildes.views.decorators import not_logged_in, rate_limit_view, use_kwargs @view_config( @@ -63,9 +62,10 @@ def finish_login(request: Request, user: User, redirect_url: str) -> HTTPFound: @use_kwargs( UserSchema( only=("username", "password"), context={"username_trim_whitespace": True} - ) + ), + location="form", ) -@use_kwargs({"from_url": String(missing="")}) +@use_kwargs({"from_url": String(missing="")}, location="form") @not_logged_in @rate_limit_view("login") def post_login( @@ -147,7 +147,9 @@ def post_login( ) @not_logged_in @rate_limit_view("login_two_factor") -@use_kwargs({"code": String(missing=""), "from_url": String(missing="")}) +@use_kwargs( + {"code": String(missing=""), "from_url": String(missing="")}, location="form" +) def post_login_two_factor(request: Request, code: str, from_url: str) -> NoReturn: """Process a log in request with 2FA.""" # Look up the user for the supplied username diff --git a/tildes/tildes/views/message.py b/tildes/tildes/views/message.py index 6822518..05f1452 100644 --- a/tildes/tildes/views/message.py +++ b/tildes/tildes/views/message.py @@ -9,10 +9,10 @@ from pyramid.request import Request from pyramid.view import view_config from sqlalchemy.dialects.postgresql import array from sqlalchemy.sql.expression import and_, or_ -from webargs.pyramidparser import use_kwargs from tildes.models.message import MessageConversation, MessageReply from tildes.schemas.message import MessageConversationSchema, MessageReplySchema +from tildes.views.decorators import use_kwargs @view_config( @@ -113,7 +113,7 @@ def get_message_conversation(request: Request) -> dict: @view_config( route_name="message_conversation", request_method="POST", permission="reply" ) -@use_kwargs(MessageReplySchema(only=("markdown",))) +@use_kwargs(MessageReplySchema(only=("markdown",)), location="form") def post_message_reply(request: Request, markdown: str) -> HTTPFound: """Post a reply to a message conversation.""" conversation = request.context @@ -129,7 +129,7 @@ def post_message_reply(request: Request, markdown: str) -> HTTPFound: @view_config(route_name="user_messages", request_method="POST", permission="message") -@use_kwargs(MessageConversationSchema(only=("subject", "markdown"))) +@use_kwargs(MessageConversationSchema(only=("subject", "markdown")), location="form") def post_user_message(request: Request, subject: str, markdown: str) -> HTTPFound: """Start a new message conversation with a user.""" new_conversation = MessageConversation( diff --git a/tildes/tildes/views/notifications.py b/tildes/tildes/views/notifications.py index 563be53..3be95cb 100644 --- a/tildes/tildes/views/notifications.py +++ b/tildes/tildes/views/notifications.py @@ -8,11 +8,11 @@ from typing import Optional from pyramid.request import Request from pyramid.view import view_config from sqlalchemy.sql.expression import desc -from webargs.pyramidparser import use_kwargs from tildes.enums import CommentLabelOption from tildes.models.comment import CommentNotification from tildes.schemas.listing import PaginatedListingSchema +from tildes.views.decorators import use_kwargs @view_config(route_name="notifications_unread", renderer="notifications_unread.jinja2") diff --git a/tildes/tildes/views/register.py b/tildes/tildes/views/register.py index a0e1f95..d8415c6 100644 --- a/tildes/tildes/views/register.py +++ b/tildes/tildes/views/register.py @@ -9,7 +9,6 @@ from pyramid.request import Request from pyramid.security import NO_PERMISSION_REQUIRED, remember from pyramid.view import view_config from sqlalchemy.exc import IntegrityError -from webargs.pyramidparser import use_kwargs from tildes.enums import LogEventType from tildes.metrics import incr_counter @@ -17,7 +16,7 @@ from tildes.models.group import Group, GroupSubscription from tildes.models.log import Log from tildes.models.user import User, UserInviteCode from tildes.schemas.user import UserSchema -from tildes.views.decorators import not_logged_in, rate_limit_view +from tildes.views.decorators import not_logged_in, rate_limit_view, use_kwargs @view_config( @@ -31,25 +30,18 @@ def get_register(request: Request, code: str) -> dict: return {"code": code} -def user_schema_check_breaches(request: Request) -> UserSchema: - """Return a UserSchema that will check the password against breaches. - - It would probably be good to generalize this function at some point, probably - similar to: - http://webargs.readthedocs.io/en/latest/advanced.html#reducing-boilerplate - """ - # pylint: disable=unused-argument - return UserSchema( - only=("username", "password"), context={"check_breached_passwords": True} - ) - - @view_config( route_name="register", request_method="POST", permission=NO_PERMISSION_REQUIRED ) -@use_kwargs(user_schema_check_breaches) @use_kwargs( - {"invite_code": String(required=True), "password_confirm": String(required=True)} + UserSchema( + only=("username", "password"), context={"check_breached_passwords": True} + ), + location="form", +) +@use_kwargs( + {"invite_code": String(required=True), "password_confirm": String(required=True)}, + location="form", ) @not_logged_in @rate_limit_view("register") diff --git a/tildes/tildes/views/settings.py b/tildes/tildes/views/settings.py index 4b703ce..e71d7bf 100644 --- a/tildes/tildes/views/settings.py +++ b/tildes/tildes/views/settings.py @@ -14,7 +14,6 @@ from pyramid.request import Request from pyramid.response import Response from pyramid.view import view_config from sqlalchemy import func -from webargs.pyramidparser import use_kwargs from tildes.enums import CommentLabelOption, CommentTreeSortOption from tildes.lib.datetime import utc_now @@ -28,6 +27,7 @@ from tildes.schemas.user import ( EMAIL_ADDRESS_NOTE_MAX_LENGTH, UserSchema, ) +from tildes.views.decorators import use_kwargs PASSWORD_FIELD = UserSchema(only=("password",)).fields["password"] @@ -139,7 +139,8 @@ def get_settings_bio(request: Request) -> dict: "old_password": PASSWORD_FIELD, "new_password": PASSWORD_FIELD, "new_password_confirm": PASSWORD_FIELD, - } + }, + location="form", ) def post_settings_password_change( request: Request, old_password: str, new_password: str, new_password_confirm: str diff --git a/tildes/tildes/views/topic.py b/tildes/tildes/views/topic.py index 7a4cd10..715d5e0 100644 --- a/tildes/tildes/views/topic.py +++ b/tildes/tildes/views/topic.py @@ -19,7 +19,6 @@ from sqlalchemy import cast from sqlalchemy.orm import joinedload from sqlalchemy.sql.expression import any_, desc from sqlalchemy_utils import Ltree -from webargs.pyramidparser import use_kwargs from tildes.enums import ( CommentLabelOption, @@ -39,7 +38,7 @@ from tildes.schemas.comment import CommentSchema from tildes.schemas.fields import Enum, ShortTimePeriod from tildes.schemas.listing import TopicListingSchema from tildes.schemas.topic import TopicSchema -from tildes.views.decorators import rate_limit_view +from tildes.views.decorators import rate_limit_view, use_kwargs from tildes.views.financials import get_financial_data @@ -47,10 +46,10 @@ DefaultSettings = namedtuple("DefaultSettings", ["order", "period"]) @view_config(route_name="group_topics", request_method="POST", permission="post_topic") -@use_kwargs(TopicSchema(only=("title", "markdown", "link"))) +@use_kwargs(TopicSchema(only=("title", "markdown", "link")), location="form") @use_kwargs( {"tags": String(missing=""), "confirm_repost": Boolean(missing=False)}, - locations=("form",), # will crash due to trying to find JSON data without this + location="form", ) def post_group_topics( request: Request, @@ -470,7 +469,7 @@ def get_topic(request: Request, comment_order: CommentTreeSortOption) -> dict: @view_config(route_name="topic", request_method="POST", permission="comment") -@use_kwargs(CommentSchema(only=("markdown",))) +@use_kwargs(CommentSchema(only=("markdown",)), location="form") @rate_limit_view("comment_post") def post_comment_on_topic(request: Request, markdown: str) -> HTTPFound: """Post a new top-level comment on a topic.""" diff --git a/tildes/tildes/views/user.py b/tildes/tildes/views/user.py index a5eb16f..2bc8162 100644 --- a/tildes/tildes/views/user.py +++ b/tildes/tildes/views/user.py @@ -10,7 +10,6 @@ from marshmallow.fields import String from pyramid.request import Request from pyramid.view import view_config from sqlalchemy.sql.expression import desc -from webargs.pyramidparser import use_kwargs from tildes.enums import CommentLabelOption, CommentSortOption, TopicSortOption from tildes.models.comment import Comment @@ -19,6 +18,7 @@ from tildes.models.topic import Topic from tildes.models.user import User, UserInviteCode from tildes.schemas.fields import PostType from tildes.schemas.listing import MixedListingSchema +from tildes.views.decorators import use_kwargs @view_config(route_name="user", renderer="user.jinja2") diff --git a/tildes/tildes/views/votes.py b/tildes/tildes/views/votes.py index 7472da9..76a25f3 100644 --- a/tildes/tildes/views/votes.py +++ b/tildes/tildes/views/votes.py @@ -5,16 +5,16 @@ from typing import Optional, Type, Union from pyramid.request import Request from pyramid.view import view_config from sqlalchemy.sql import desc -from webargs.pyramidparser import use_kwargs from tildes.models.comment import Comment, CommentVote from tildes.models.topic import Topic, TopicVote from tildes.schemas.fields import PostType from tildes.schemas.listing import PaginatedListingSchema +from tildes.views.decorators import use_kwargs @view_config(route_name="votes", renderer="votes.jinja2") -@use_kwargs(PaginatedListingSchema) +@use_kwargs(PaginatedListingSchema()) @use_kwargs({"post_type": PostType(data_key="type", missing="topic")}) def get_voted_posts( request: Request,