Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve from search prefix error handling #37

Merged
merged 3 commits into from
Apr 9, 2022
Merged

Improve from search prefix error handling #37

merged 3 commits into from
Apr 9, 2022

Conversation

single-right-quote
Copy link
Contributor

@single-right-quote single-right-quote commented Apr 5, 2022 •
edited
Loading

addresses #36

note mastodon/mastodon#17963; i expect they will ask for adjustments there; i further expect to make impactful design changes. i am still opening this PR as we are currently running this on GlitchCat anyway, with no reported problems

@single-right-quote
Make from search prefix more robust (addresses mastodon/mastodon#17941
db8fcc6 
)

* Improve robustness for account string validation

Using unsupported prefixes now reports a 422; searching for posts from an
account the instance is not aware of reports a 404. TODO: The UI for this
on the front end is abysmal.

Searching `from:username@domain` now succeeds when `domain` is the local
domain; searching `from:@username(@Domain)?` now works as expected.

* Satisfy upstream rubcocp

* Unbreak upstream tests
@single-right-quote
Copy link
Contributor Author

updated for upstream rubocop and upstream tests

@single-right-quote
Copy link
Contributor Author

single-right-quote commented Apr 8, 2022 •
edited
Loading

we will not be incorporating upstream’s requested changes as i prefer to alert the user to syntactically invalid account strings; as well as to reject account strings like [email protected]@anthuesaho, which the upstream PR will accept

@single-right-quote single-right-quote changed the title Make from search prefix more robust (addresses mastodon/mastodon#17941) Improve from search prefix error handling (addresses mastodon/mastodon#17941) Apr 8, 2022
@single-right-quote single-right-quote changed the title Improve from search prefix error handling (addresses mastodon/mastodon#17941) Improve from search prefix error handling (addresses mastodon/mastodon#17941) #36 Apr 8, 2022
@single-right-quote single-right-quote changed the title Improve from search prefix error handling (addresses mastodon/mastodon#17941) #36 Improve from search prefix error handling Apr 8, 2022
@single-right-quote single-right-quote mentioned this pull request Apr 9, 2022
Merged
marrus-sh
marrus-sh reviewed Apr 9, 2022
View reviewed changes
app/controllers/api/v2/search_controller.rb Outdated Show resolved Hide resolved
app/lib/search_query_transformer.rb Outdated Show resolved Hide resolved
app/models/concerns/account_finder_concern.rb Outdated Show resolved Hide resolved
@marrus-sh
Copy link
Member

It’d be nice to have tests for at least the new methods, so that if something upstream breaks this we would have some manner of indication. I wouldn’t rate it as an especially high priority though.

@single-right-quote
Make account string validation consistent with mention processing
04087db 
We previously matched on one-character domains and domains ending with
`[\.-]`, allowing `from:@A@a` and `from:@A@a-` searches to cause an
account lookup. This commit will raise a syntax error in both cases, as
MENTION_RE would never match them.
@single-right-quote
Refactor from prefix error handling.
f537f1d 
Incorporates changes suggested in #37. In doing so, adopts an error
handling style more consistent with the existing codebase (for which I
must thank @ClearlyClaire).

Removes new code no longer in use.
@single-right-quote
Copy link
Contributor Author

single-right-quote commented Apr 9, 2022 •
edited
Loading

i really should add tests but my smoke tests (running these commits briefly on GlitchCat and testing that searches work, and and that searchs 422 on one-character domain names or domain names ending not in :word:) raised no problems

(we really need to set up a test instance)

@single-right-quote single-right-quote linked an issue Apr 9, 2022 that may be closed by this pull request
from: search prefix 500s when the supplied account cannot be found #36
Closed
@single-right-quote single-right-quote added STATUS: bug Improper behaviour LV.1: Application Backend @ installing and running the application labels Apr 9, 2022
marrus-sh
marrus-sh approved these changes Apr 9, 2022
View reviewed changes
Copy link
Member

@marrus-sh marrus-sh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Feel free to merge if you don’t want to bother with tests yet; maybe at least run rubocop though if you haven’t since we don’t have CI.

@single-right-quote
Copy link
Contributor Author

rubocop reports no (new) offenses; i’m gonna merge and then immediately file an issue to write tests, since we really ought to

@single-right-quote single-right-quote merged commit b8e801a into main Apr 9, 2022
@single-right-quote single-right-quote deleted the more-robust-from-prefix branch April 9, 2022 23:39
@single-right-quote single-right-quote mentioned this pull request Apr 9, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marrus-sh marrus-sh marrus-sh approved these changes

Assignees
No one assigned
Labels
LV.1: Application Backend @ installing and running the application STATUS: bug Improper behaviour
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

from: search prefix 500s when the supplied account cannot be found
2 participants
@single-right-quote @marrus-sh