Merge pull request #44 from CaptainFact/fix/42-url-length

Increase sources max URL length to 2048

apps/cf/lib/comments/comments.ex

@@ -33,19 +33,18 @@ defmodule CF.Comments do
     UserPermissions.check!(user, :create, :comment)
     source_url = source_url && Source.prepare_url(source_url)
 
+    # Load source from DB or create a changeset to make a new one
     source =
       source_url &&
-        (Repo.get_by(Source, url: source_url) || Source.changeset(%Source{}, %{url: source_url}))
+        (Sources.get_by_url(source_url) || Source.changeset(%Source{}, %{url: source_url}))
 
-    comment_changeset =
+    # Insert comment in DB
+    full_comment =
       user
       |> Ecto.build_assoc(:comments)
       |> Ecto.Changeset.change()
       |> Ecto.Changeset.put_assoc(:source, source)
       |> Comment.changeset(params)
-
-    full_comment =
-      comment_changeset
       |> Repo.insert!()
       |> Map.put(:user, user)
       |> Repo.preload(:source)

apps/cf/lib/sources/sources.ex

@@ -1,9 +1,26 @@
 defmodule CF.Sources do
+  @moduledoc """
+  Functions to manage sources and their metadata.
+  """
+
   alias DB.Repo
   alias DB.Schema.Source
 
   alias CF.Sources.Fetcher
 
+  @doc """
+  Get a source from `DB` from its URL. Returns nil if no source exist for
+  this URL.
+  """
+  @spec get_by_url(binary()) :: Source.t() | nil
+  def get_by_url(url) do
+    Repo.get_by(Source, url: url)
+  end
+
+  @doc """
+  Fetch a source metadata using `CF.Sources.Fetcher`, update source with it then
+  call `callback` (if any) with the update source.
+  """
   def update_source_metadata(base_source = %Source{}, callback \\ nil) do
     Fetcher.fetch_source_metadata(base_source.url, fn
       metadata when metadata == %{} ->

apps/cf/test/cf/sources/sources_test.exs

@@ -0,0 +1,2 @@
+defmodule CF.SourcesTest do
+end

apps/db/lib/db_schema/source.ex

@@ -12,23 +12,29 @@ defmodule DB.Schema.Source do
     timestamps()
   end
 
+  @url_max_length 2048
+
   # Allow to add localhost urls as sources during tests
   @url_regex if Application.get_env(:db, :env) == :test,
                do:
                  ~r/(^https?:\/\/[-a-zA-Z0-9@:%._\+~#=]{2,256}\.[a-z]{2,6}\b([-a-zA-Z0-9@:%_\+.~#?&\/\/=]*))|localhost/,
                else:
                  ~r/^https?:\/\/[-a-zA-Z0-9@:%._\+~#=]{2,256}\.[a-z]{2,6}\b([-a-zA-Z0-9@:%_\+.~#?&\/\/=]*)/
 
+  @doc """
+  Get max URL length.
+  See https://boutell.com/newfaq/misc/urllength.html
+  """
+  def url_max_length, do: @url_max_length
+
   @doc """
   Builds a changeset based on the `struct` and `params`.
   """
   def changeset(struct, params \\ %{}) do
     struct
     |> cast(params, [:url])
     |> update_change(:url, &prepare_url/1)
-    |> validate_required([:url])
-    |> unique_constraint(:url)
-    |> validate_format(:url, @url_regex)
+    |> changeset_common_validations()
   end
 
   def changeset_fetched(struct, params) do
@@ -39,9 +45,7 @@ defmodule DB.Schema.Source do
     |> update_change(:title, &clean_and_truncate/1)
     |> update_change(:language, &String.trim/1)
     |> update_change(:site_name, &clean_and_truncate/1)
-    |> validate_required([:url])
-    |> unique_constraint(:url)
-    |> validate_format(:url, @url_regex)
+    |> changeset_common_validations()
   end
 
   @regex_contains_http ~r/^https?:\/\//
@@ -50,6 +54,14 @@ defmodule DB.Schema.Source do
     if Regex.match?(@regex_contains_http, str), do: str, else: "https://" <> str
   end
 
+  defp changeset_common_validations(changeset) do
+    changeset
+    |> validate_required([:url])
+    |> unique_constraint(:url)
+    |> validate_format(:url, @url_regex)
+    |> validate_length(:url, min: 10, max: @url_max_length)
+  end
+
   defp clean_and_truncate(str) do
     if String.valid?(str) do
       str = DB.Utils.String.trim_all_whitespaces(str)

apps/db/priv/repo/migrations/20181109223648_increate_source_max_url_length.exs

@@ -0,0 +1,19 @@
+defmodule DB.Repo.Migrations.IncreateSourceMaxUrlLength do
+  use Ecto.Migration
+
+  def up do
+    alter table(:sources) do
+      modify(:url, :string, size: 2048)
+    end
+  end
+
+  def down do
+    Ecto.Adapters.SQL.query!(DB.Repo, """
+    DELETE FROM sources WHERE LENGTH(url) > 255;
+    """)
+
+    alter table(:sources) do
+      modify(:url, :string)
+    end
+  end
+end

apps/db/test/db_schema/source_test.exs

@@ -16,12 +16,31 @@ defmodule DB.Schema.SourceTest do
   end
 
   test "changeset with invalid attributes" do
-    changeset = Source.changeset(%Source{}, @invalid_attrs)
-    refute changeset.valid?
+    refute Source.changeset(%Source{}, @invalid_attrs).valid?
+    refute Source.changeset(%Source{}, %{@valid_attrs | url: ""}).valid?
+    refute Source.changeset(%Source{}, %{@valid_attrs | url: "http://"}).valid?
+    refute Source.changeset(%Source{}, %{@valid_attrs | url: "https://x"}).valid?
+    refute Source.changeset(%Source{}, %{@valid_attrs | url: "https://xxxxxx"}).valid?
   end
 
   test "must add https:// if url doesn't start with http:// or https://" do
     changeset = Source.changeset(%Source{}, Map.put(@valid_attrs, :url, "amazing.com/article"))
     assert changeset.changes.url == "https://amazing.com/article"
   end
+
+  test "should not accept URLs longer than 2048 characters" do
+    attrs = %{@valid_attrs | url: url_generator(2049)}
+    refute Source.changeset(%Source{}, attrs).valid?
+    expected_error = {:url, "should be at most 2048 character(s)"}
+    assert expected_error in errors_on(%Source{}, attrs)
+  end
+
+  @url_generator_base "https://captainfact.io/"
+  defp url_generator(length) do
+    @url_generator_base <>
+      (fn -> "x" end
+       |> Stream.repeatedly()
+       |> Enum.take(length - String.length(@url_generator_base))
+       |> Enum.join())
+  end
 end