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

Prevent search for invalid queries #34

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Prevent search for invalid queries #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a1a7be
add tests
aenglisc Jan 21, 2020
c7580be
add handling
aenglisc Jan 21, 2020
3a0c0fe
Merge branch 'master' into prevent_search_for_invalid_queries
aenglisc Jan 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions lib/diff_web/live/search_view.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,27 @@ defmodule DiffWeb.SearchLiveView do
DiffWeb.SearchView.render("search.html", assigns)
end

@valid_query ~r{^[a-zA-Z0-9_]+$}
Copy link
Member

Choose a reason for hiding this comment

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

We should use this regex: https://github.com/hexpm/hexpm/blob/master/lib/hexpm/repository/package.ex#L67.

Copy link
Author

@aenglisc aenglisc Jan 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Should we though? To me a query that starts with a number (say, 3339) or a capital letter seems fairly legitimate. 🤔

Copy link
Collaborator

@joladev joladev Jan 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't feel that validating the queries is an improvement here. Looking up package names is not an expensive operation, it's easier to just let users search for what they want. And suggestions will still work, even if the query includes characters that are not allowed in Hex package names, eg typing in "pho!nix" will suggest "phoenix".

Copy link
Author

@aenglisc aenglisc Jan 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

And suggestions will still work, even if the query includes characters that are not allowed in Hex package names, eg typing in "pho!nix" will suggest "phoenix".

That's a good point. The reason why this idea came to me in the first place was that it's not uncommon for people with non-latin keyboards (including myself) to forget to switch, thus, say phoenix would become зрщутшч, ending up in multiple completely wasted search attempts. Perhaps adding some kind of filter would be a better option. Or maybe something even more exciting, such as a mapping mechanism that would transform the aforementioned зрщутшч back into phoenix? Although the latter would seem fairly unrealistic given the amount of languages in the world.


def mount(_session, socket) do
{:ok, reset_state(socket)}
end

def handle_event("search", %{"q" => ""}, socket), do: {:noreply, reset_state(socket)}

def handle_event("search", %{"q" => query}, socket)
when byte_size(query) > 30 do
{:noreply, socket}
end

def handle_event("search", %{"q" => query}, socket) do
query = String.downcase(query)
send(self(), {:search, query})
{:noreply, assign(socket, query: query)}
if String.match?(query, @valid_query) do
query = String.downcase(query)
send(self(), {:search, query})
{:noreply, assign(socket, query: query)}
else
{:noreply, socket}
end
end

def handle_event("search_" <> suggestion, _params, socket) do
Expand All @@ -30,22 +41,15 @@ defmodule DiffWeb.SearchLiveView do
index_of_selected_from = Enum.find_index(releases, &(&1 == from))
to_releases = Enum.slice(releases, (index_of_selected_from + 1)..-1)

{:noreply,
assign(socket,
from: from,
to_releases: to_releases
)}
{:noreply, assign(socket, from: from, to_releases: to_releases)}
end

def handle_event(
"select_version",
%{"_target" => ["to"], "to" => to},
socket
) do
{:noreply,
assign(socket,
to: to
)}
{:noreply, assign(socket, to: to)}
end

def handle_event("go", _params, %{assigns: %{result: result, to: to, from: from}} = socket)
Expand Down
30 changes: 30 additions & 0 deletions test/diff_web/live/search_view_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,36 @@ defmodule DiffWeb.SearchLiveViewTest do
refute rendered =~ ~s(Package phoenix not found.)
end

test "do not search if the query is too long", %{conn: conn} do
{:ok, view, _html} = live(conn, "/")

Diff.Package.StoreMock
|> expect(:get_names, fn -> ["phoenix", "phoenix_live_view"] end)
|> expect(:get_versions, fn "phoenix" -> {:ok, ["1.4.10", "1.4.11"]} end)
|> allow(self(), view.pid)

send(view.pid, {:search, "phoenix"})
rendered = render(view)

assert rendered =~
render_change(view, "search", %{"q" => "phoenix_phoenix_phoenix_phoenix_phoenix"})
end

test "do not search if the query has invalid characters", %{conn: conn} do
{:ok, view, _html} = live(conn, "/")

Diff.Package.StoreMock
|> expect(:get_names, fn -> ["phoenix", "phoenix_live_view"] end)
|> expect(:get_versions, fn "phoenix" -> {:ok, ["1.4.10", "1.4.11"]} end)
|> allow(self(), view.pid)

send(view.pid, {:search, "phoenix"})
rendered = render(view)

assert rendered =~ render_change(view, "search", %{"q" => "phoenix-"})
assert rendered =~ render_change(view, "search", %{"q" => "phoenixё"})
end

test "clicking diff", %{conn: conn} do
{:ok, view, _html} = live(conn, "/")

Expand Down