Repository: phoenixframework/phoenix
Pull request #5284 (2023-02-17 11:18:29)

Add comments to gen.auth schema template


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index aa8b910f0..c99776178 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -35,6 +35,11 @@ This feature replaces the `Helpers` module generated in your Phoenix router, but
 will continue to work and be generated. You can disable router helpers by passing the
 `helpers: false` option to `use Phoenix.Router`.
 
+## 1.7.0-rc.4
+
+### Bug Fixes
+  * Fix race conditions in the longpoll trasnport by batching messages
+
 ## 1.7.0-rc.3 (2023-02-15)
 
 ### Enhancements
diff --git a/assets/js/phoenix/longpoll.js b/assets/js/phoenix/longpoll.js
index 18c15a2fc..ff3c57a9f 100644
--- a/assets/js/phoenix/longpoll.js
+++ b/assets/js/phoenix/longpoll.js
@@ -12,6 +12,10 @@ export default class LongPoll {
     this.token = null
     this.skipHeartbeat = true
     this.reqs = new Set()
+    this.awaitingBatchAck = false
+    this.currentBatch = null
+    this.currentBatchTimer = null
+    this.batchBuffer = []
     this.onopen = function (){ } // noop
     this.onerror = function (){ } // noop
     this.onmessage = function (){ } // noop
@@ -45,7 +49,7 @@ export default class LongPoll {
   isActive(){ return this.readyState === SOCKET_STATES.open || this.readyState === SOCKET_STATES.connecting }
 
   poll(){
-    this.ajax("GET", null, () => this.ontimeout(), resp => {
+    this.ajax("GET", "application/json", null, () => this.ontimeout(), resp => {
       if(resp){
         var {status, token, messages} = resp
         this.token = token
@@ -100,11 +104,33 @@ export default class LongPoll {
     })
   }
 
+  // we collect all pushes within the current event loop by
+  // setTimeout 0, which optimizes back-to-back procedural
+  // pushes against an empty buffer
   send(body){
-    this.ajax("POST", body, () => this.onerror("timeout"), resp => {
+    if(this.currentBatch){
+      this.currentBatch.push(body)
+    } else if(this.awaitingBatchAck){
+      this.batchBuffer.push(body)
+    } else {
+      this.currentBatch = [body]
+      this.currentBatchTimer = setTimeout(() => {
+        this.batchSend(this.currentBatch)
+        this.currentBatch = null
+      }, 0)
+    }
+  }
+
+  batchSend(messages){
+    this.awaitingBatchAck = true
+    this.ajax("POST", "application/x-ndjson", messages.join("\n"), () => this.onerror("timeout"), resp => {
+      this.awaitingBatchAck = false
       if(!resp || resp.status !== 200){
         this.onerror(resp && resp.status)
         this.closeAndRetry(1011, "internal server error", false)
+      } else if(this.batchBuffer.length > 0){
+        this.batchSend(this.batchBuffer)
+        this.batchBuffer = []
       }
     })
   }
@@ -113,6 +139,9 @@ export default class LongPoll {
     for(let req of this.reqs){ req.abort() }
     this.readyState = SOCKET_STATES.closed
     let opts = Object.assign({code: 1000, reason: undefined, wasClean: true}, {code, reason, wasClean})
+    this.batchBuffer = []
+    clearTimeout(this.currentBatchTimer)
+    this.currentBatchTimer = null
     if(typeof(CloseEvent) !== "undefined"){
       this.onclose(new CloseEvent("close", opts))
     } else {
@@ -120,13 +149,13 @@ export default class LongPoll {
     }
   }
 
-  ajax(method, body, onCallerTimeout, callback){
+  ajax(method, contentType, body, onCallerTimeout, callback){
     let req
     let ontimeout = () => {
       this.reqs.delete(req)
       onCallerTimeout()
     }
-    req = Ajax.request(method, this.endpointURL(), "application/json", body, this.timeout, ontimeout, resp => {
+    req = Ajax.request(method, this.endpointURL(), contentType, body, this.timeout, ontimeout, resp => {
       this.reqs.delete(req)
       if(this.isActive()){ callback(resp) }
     })
diff --git a/guides/components.md b/guides/components.md
index 31e1ce95a..af07635bc 100644
--- a/guides/components.md
+++ b/guides/components.md
@@ -34,7 +34,7 @@ That's simple enough. There's only two lines, `use HelloWeb, :html`. This line c
 
 All of the imports and aliases we make in our module will also be available in our templates. That's because templates are effectively compiled into functions inside their respective module. For example, if you define a function in your module, you will be able to invoke it directly from the template. Let's see this in practice.
 
-Imagine we want to refactor our `show.html.heex` to move the rendering of `<h2>Hello World, from <%= @messenger %>!</h2>` to its own function. We can move it to a function component inside `HelloHTML`:
+Imagine we want to refactor our `show.html.heex` to move the rendering of `<h2>Hello World, from <%= @messenger %>!</h2>` to its own function. We can move it to a function component inside `HelloHTML`, let's do so:
 
 ```elixir
 defmodule HelloWeb.HelloHTML do
@@ -52,7 +52,7 @@ defmodule HelloWeb.HelloHTML do
 end
 ```
 
-We declared the attributes we accept via `attr` provided by `Phoenix.Component`, then we defined our `title/1` function which returns the HEEx template. When we reload our home page, we should see our new title. Since templates are compiled inside the view, we can invoke the view function simply as `<.greet messenger="..." />`, but we can also type `<HelloWeb.HelloHTML.greet messenger="..." />` if the component was defined elsewhere.
+In the example above, we defined a `greet/1` function which returns the HEEx template. Above the function, we called `attr`, provided by `Phoenix.Component`, which defines the attributes/assigns that function expects. Since templates are embedded inside the `HelloHTML` module, we can invoke the our component simply as `<.greet messenger="..." />`, but we can also type `<HelloWeb.HelloHTML.greet messenger="..." />` if the component was defined elsewhere.
 
 By declaring attributes, Phoenix will warn if we call the `<.greet />` component without passing attributes. If an attribute is optional, you can specify the `:default` option with a value:
 
@@ -183,13 +183,13 @@ In other words, after rendering your page, the result is placed in the `@inner_c
 
 Phoenix provides all kinds of conveniences to control which layout should be rendered. For example, the `Phoenix.Controller` module provides the `put_root_layout/2` function for us to switch _root layouts_. This takes `conn` as its first argument and a keyword list of formats and their layouts. You can set it to `false` to disable the layout altogether.
 
-You can edit the `index` action of `PageController` in `lib/hello_web/controllers/page_controller.ex` to look like this.
+You can edit the `home` action of `PageController` in `lib/hello_web/controllers/page_controller.ex` to look like this.
 
 ```elixir
-def index(conn, _params) do
+def home(conn, _params) do
   conn
   |> put_root_layout(html: false)
-  |> render(:index)
+  |> render(:home)
 end
 ```
 
@@ -197,13 +197,13 @@ After reloading [http://localhost:4000/](http://localhost:4000/), we should see
 
 To customize the application layout, we invoke a similar function named `put_layout/2`. Let's actually create another layout and render the index template into it. As an example, let's say we had a different layout for the admin section of our application which didn't have the logo image. To do this, copy the existing `app.html.heex` to a new file `admin.html.heex` in the same directory `lib/hello_web/components/layouts`. Then remove everything inside the `<header>...</header>` tags (or change it to whatever you desire) in the new file.
 
-Now, in the `index` action of the controller of `lib/hello_web/controllers/page_controller.ex`, add the following:
+Now, in the `home` action of the controller of `lib/hello_web/controllers/page_controller.ex`, add the following:
 
 ```elixir
-def index(conn, _params) do
+def home(conn, _params) do
   conn
   |> put_layout(html: :admin)
-  |> render(:index)
+  |> render(:home)
 end
 ```
 
diff --git a/installer/lib/phx_new/generator.ex b/installer/lib/phx_new/generator.ex
index 015f8c52e..64f201a7a 100644
--- a/installer/lib/phx_new/generator.ex
+++ b/installer/lib/phx_new/generator.ex
@@ -356,7 +356,8 @@ defmodule Phx.New.Generator do
           For example: ecto://USER:PASS@HOST/DATABASE
           \"""
 
-      maybe_ipv6 = if System.get_env("ECTO_IPV6"), do: [:inet6], else: []
+      maybe_ipv6 = if System.get_env("ECTO_IPV6") in ~w(true 1), do: [:inet6], else: []
+
       """,
       prod_config: """
       # ssl: true,
diff --git a/lib/mix/tasks/phx.gen.release.ex b/lib/mix/tasks/phx.gen.release.ex
index e0cf66944..dc97c567e 100644
--- a/lib/mix/tasks/phx.gen.release.ex
+++ b/lib/mix/tasks/phx.gen.release.ex
@@ -113,7 +113,7 @@ defmodule Mix.Tasks.Phx.Gen.Release do
 
       Add the following to your config/runtime.exs:
 
-          maybe_ipv6 = if System.get_env("ECTO_IPV6"), do: [:inet6], else: []
+          maybe_ipv6 = if System.get_env("ECTO_IPV6") in ~w(true 1), do: [:inet6], else: []
 
           config :#{app}, #{app_namespace}.Repo,
             ...,
diff --git a/lib/phoenix/transports/long_poll.ex b/lib/phoenix/transports/long_poll.ex
index 135df8137..6628d6bbf 100644
--- a/lib/phoenix/transports/long_poll.ex
+++ b/lib/phoenix/transports/long_poll.ex
@@ -72,7 +72,21 @@ defmodule Phoenix.Transports.LongPoll do
   defp publish(conn, server_ref, endpoint, opts) do
     case read_body(conn, []) do
       {:ok, body, conn} ->
-        status = transport_dispatch(endpoint, server_ref, body, opts)
+        # we need to match on both v1 and v2 protocol, as well as wrap for backwards compat
+        batch =
+          case get_req_header(conn, "content-type") do
+            ["application/x-ndjson"] -> String.split(body, ["\n", "\r\n"])
+            _ -> [body]
+          end
+
+        {conn, status} =
+          Enum.reduce_while(batch, {conn, nil}, fn msg, {conn, _status} ->
+            case transport_dispatch(endpoint, server_ref, msg, opts) do
+              :ok -> {:cont, {conn, :ok}}
+              :request_timeout = timeout -> {:halt, {conn, timeout}}
+            end
+          end)
+
         conn |> put_status(status) |> status_json()
 
       _ ->
@@ -168,8 +182,12 @@ defmodule Phoenix.Transports.LongPoll do
 
   ## Helpers
 
-  defp server_ref(endpoint_id, id, pid, topic) do
-    if endpoint_id == id and Process.alive?(pid), do: pid, else: topic
+  defp server_ref(endpoint_id, id, pid, topic) when is_pid(pid) do
+    cond do
+      node(pid) in Node.list() -> pid
+      endpoint_id == id and Process.alive?(pid) -> pid
+      true -> topic
+    end
   end
 
   defp client_ref(topic) when is_binary(topic), do: topic
diff --git a/priv/templates/phx.gen.auth/auth.ex b/priv/templates/phx.gen.auth/auth.ex
index 6dcf8ff68..9aa6a753e 100644
--- a/priv/templates/phx.gen.auth/auth.ex
+++ b/priv/templates/phx.gen.auth/auth.ex
@@ -173,15 +173,11 @@ defmodule <%= inspect auth_module %> do
   end
 
   defp mount_current_<%= schema.singular %>(session, socket) do
-    case session do
-      %{"<%= schema.singular %>_token" => <%= schema.singular %>_token} ->
-        Phoenix.Component.assign_new(socket, :current_<%= schema.singular %>, fn ->
-          <%= inspect context.alias %>.get_<%= schema.singular %>_by_session_token(<%= schema.singular %>_token)
-        end)
-
-      %{} ->
-        Phoenix.Component.assign_new(socket, :current_<%= schema.singular %>, fn -> nil end)
-    end
+    Phoenix.Component.assign_new(socket, :current_<%= schema.singular %>, fn ->
+      if <%= schema.singular %>_token = session["<%= schema.singular %>_token"] do
+        <%= inspect context.alias %>.get_<%= schema.singular %>_by_session_token(<%= schema.singular %>_token)
+      end
+    end)
   end
 
   @doc """
diff --git a/priv/templates/phx.gen.auth/schema.ex b/priv/templates/phx.gen.auth/schema.ex
index 81a33e83a..2624e96f3 100644
--- a/priv/templates/phx.gen.auth/schema.ex
+++ b/priv/templates/phx.gen.auth/schema.ex
@@ -54,6 +54,7 @@ defmodule <%= inspect schema.module %> do
     changeset
     |> validate_required([:password])
     |> validate_length(:password, min: 12, max: 72)
+    # Examples of additional password validation:
     # |> validate_format(:password, ~r/[a-z]/, message: "at least one lower case character")
     # |> validate_format(:password, ~r/[A-Z]/, message: "at least one upper case character")
     # |> validate_format(:password, ~r/[!?@#$%^&*_0-9]/, message: "at least one digit or punctuation character")
@@ -68,6 +69,8 @@ defmodule <%= inspect schema.module %> do
       changeset<%= if hashing_library.name == :bcrypt do %>
       # If using Bcrypt, then further validate it is at most 72 bytes long
       |> validate_length(:password, max: 72, count: :bytes)<% end %>
+      # Hashing could be done with `Ecto.Changeset.prepare_changes/2`, but that
+      # would keep the database transaction open longer and hurt performance.
       |> put_change(:hashed_password, <%= inspect hashing_library.module %>.hash_pwd_salt(password))
       |> delete_change(:password)
     else


Test patch:
diff --git a/installer/test/phx_new_test.exs b/installer/test/phx_new_test.exs
index f73d11fe9..9600d3320 100644
--- a/installer/test/phx_new_test.exs
+++ b/installer/test/phx_new_test.exs
@@ -178,7 +178,10 @@ defmodule Mix.Tasks.Phx.NewTest do
 
       assert_file("phx_blog/config/runtime.exs", fn file ->
         assert file =~ config
-        assert file =~ ~S|maybe_ipv6 = if System.get_env("ECTO_IPV6"), do: [:inet6], else: []|
+
+        assert file =~
+                 ~S|maybe_ipv6 = if System.get_env("ECTO_IPV6") in ~w(true 1), do: [:inet6], else: []|
+
         assert file =~ ~S|socket_options: maybe_ipv6|
 
         assert file =~ """
diff --git a/test/phoenix/integration/long_poll_channels_test.exs b/test/phoenix/integration/long_poll_channels_test.exs
index aa4da8d36..92e5e35d2 100644
--- a/test/phoenix/integration/long_poll_channels_test.exs
+++ b/test/phoenix/integration/long_poll_channels_test.exs
@@ -159,7 +159,13 @@ defmodule Phoenix.Integration.LongPollChannelsTest do
   """
   def poll(method, path, vsn, params, json \\ nil, headers \\ %{}) do
     {serializer, json} = serializer(vsn, json)
-    headers = Map.merge(%{"content-type" => "application/json"}, headers)
+    headers =
+      if is_list(json) do
+        Map.merge(%{"content-type" => "application/x-ndjson"}, headers)
+      else
+        Map.merge(%{"content-type" => "application/json"}, headers)
+      end
+
     body = encode(serializer, json)
     query_string = params |> Map.put("vsn", vsn) |> URI.encode_query()
     url = "http://127.0.0.1:#{@port}#{path}/longpoll?" <> query_string
@@ -168,8 +174,12 @@ defmodule Phoenix.Integration.LongPollChannelsTest do
   end
 
   defp serializer("2." <> _, json), do: {V2.JSONSerializer, json}
-  defp serializer(_, json) do
-    {V1.JSONSerializer, json && Map.delete(json, "join_ref")}
+  defp serializer(_, nil), do: {V1.JSONSerializer, nil}
+  defp serializer(_, batch) when is_list(batch) do
+    {V1.JSONSerializer, for(msg <- batch, do: Map.delete(msg, "join_ref"))}
+  end
+  defp serializer(_, %{} = json) do
+    {V1.JSONSerializer, json}
   end
 
   defp decode_body(serializer, %{} = resp) do
@@ -184,13 +194,19 @@ defmodule Phoenix.Integration.LongPollChannelsTest do
 
   defp encode(_vsn, nil), do: ""
 
-  defp encode(V2.JSONSerializer, map) do
+  defp encode(V2.JSONSerializer = serializer, batch) when is_list(batch) do
+    batch
+    |> Enum.map(&encode(serializer, &1))
+    |> Enum.join("\n")
+  end
+
+  defp encode(V2.JSONSerializer, %{} = map) do
     Phoenix.json_library().encode!(
       [map["join_ref"], map["ref"], map["topic"], map["event"], map["payload"]]
     )
   end
 
-  defp encode(V1.JSONSerializer, map), do: Phoenix.json_library().encode!(map)
+  defp encode(V1.JSONSerializer, %{} = map), do: Phoenix.json_library().encode!(map)
 
   @doc """
   Joins a long poll socket.
@@ -385,6 +401,59 @@ defmodule Phoenix.Integration.LongPollChannelsTest do
       end
     end
 
+    test "#{@mode}: lonpoll publishing batch events on v2 protocol" do
+      vsn = "2.0.0"
+      Phoenix.PubSub.subscribe(__MODULE__, "room:lobby")
+      session = join("/ws", "room:lobby", vsn, "1", @mode)
+      # Publish successfully
+      resp =
+        poll(:post, "/ws", vsn, session, [
+          %{
+            "topic" => "room:lobby",
+            "event" => "new_msg",
+            "ref" => "2",
+            "join_ref" => "1",
+            "payload" => %{"body" => "hi1"}
+          },
+          %{
+            "topic" => "room:lobby",
+            "event" => "new_msg",
+            "ref" => "3",
+            "join_ref" => "1",
+            "payload" => %{"body" => "hi2"}
+          }
+        ])
+
+
+      assert resp.body["status"] == 200
+      assert_receive %Broadcast{event: "new_msg", payload: %{"body" => "hi1"}}
+      assert_receive %Broadcast{event: "new_msg", payload: %{"body" => "hi2"}}
+
+      # Get published message
+      resp = poll(:get, "/ws", vsn, session)
+      assert resp.body["status"] == 200
+
+      assert [
+               _phx_reply,
+               _user_entered,
+               _joined,
+               %Message{
+                 topic: "room:lobby",
+                 event: "new_msg",
+                 payload: %{"body" => "hi1", "transport" => ":longpoll"},
+                 ref: nil,
+                 join_ref: "1"
+               },
+               %Message{
+                 topic: "room:lobby",
+                 event: "new_msg",
+                 payload: %{"body" => "hi2", "transport" => ":longpoll"},
+                 ref: nil,
+                 join_ref: "1"
+               }
+             ] = resp.body["messages"]
+    end
+
     test "#{@mode}: shuts down after timeout" do
       session = join("/ws", "room:lobby", @vsn, "1", @mode)