pow-auth / pow_assent

Multi-provider authentication for your Pow enabled app
https://powauth.com
MIT License
318 stars 50 forks source link

0.4.14 -> 0.4.17 compilation errors around `render_form` #240

Closed coladarci closed 1 year ago

coladarci commented 1 year ago

When trying to upgrade from 0.4.14 to 0.4.17 I'm getting compilation errors:

 == Compilation error in file lib/pow_assent/phoenix/controllers/registration_html.ex ==
Error: ** (CompileError) nofile:3: undefined function render_form/1 (there is no such import)
    (pow 1.0.27) expanding macro: Pow.Phoenix.Template.template/3

Relevant Deps are:

* phoenix 1.7.2 (Hex package) (mix)
  locked at 1.7.2 (phoenix) 1ebca94b
  ok
* phoenix_ecto 4.4.0 (Hex package) (mix)
  locked at 4.4.0 (phoenix_ecto) 09864e55
  ok
* phoenix_html 3.3.1 (Hex package) (mix)
  locked at 3.3.1 (phoenix_html) bed1906e
  ok
* phoenix_live_dashboard 0.7.2 (Hex package) (mix)
  locked at 0.7.2 (phoenix_live_dashboard) 0e5fdf06
  ok
* phoenix_live_reload 1.4.1 (Hex package) (mix)
  locked at 1.4.1 (phoenix_live_reload) 9bffb834
  ok
* phoenix_live_view 0.18.18 (Hex package) (mix)
  locked at 0.18.18 (phoenix_live_view) a5810d04
  ok
* phoenix_pubsub 2.1.1 (Hex package) (mix)
  locked at 2.1.1 (phoenix_pubsub) 81367c6d
  ok
* phoenix_template 1.0.1 (Hex package) (mix)
  locked at 1.0.1 (phoenix_template) 157dc078
  ok
* phoenix_view 2.0.2 (Hex package) (mix)
  locked at 2.0.2 (phoenix_view) a929e723
  ok

Curious if you have any thoughts..

Thanks!

danschultzer commented 1 year ago

Did you upgrade from 1.6 to 1.7 at the same time? Try remove the _build directory and rebuild, might be compile dependency issue.

coladarci commented 1 year ago

Interesting - so this was happening in CI; I blew away all caching and it happened again. I pulled down the branch, removed the _build, still happening. I can trace it to here https://github.com/pow-auth/pow_assent/pull/235/files#diff-005dd854f4b5bffbd5a28e1a808eddbe7a4b559f210e51b49f1670aefc7d05e5R5 - if I go into a working 0.4.14 build, I can verify the logic is working

iex> Pow.dependency_vsn_match?(:phoenix, ">= 1.7.0")
true

I'm sort of at a loss of what to try next...

danschultzer commented 1 year ago

When you compile your project in local, does it seem that Pow/PowAssent is compiled before Phoenix? Is your project umbrella or a single app phoenix? I've got a feeling this is the dependency tree that's somehow compiling Pow/PowAssent before Phoenix.

It also makes me think that I should change the logic to something like Pow.dependency_vsn_match?(:phoenix, "< 1.7.0"), as it seems better to expect that user is on latest release rather than the other way around.

coladarci commented 1 year ago

It's a single app. It appears the order is fine - when I do a fresh compile this is the order:

==> ecto
==> params
==> ecto_sql
==> ecto_hashids
==> assent
==> ex_money_sql
==> websock
==> websock_adapter
==> phoenix
==> bamboo
==> bamboo_smtp
==> new_relic_agent
==> phoenix_live_reload
==> phoenix_live_view
==> phoenix_live_dashboard
==> heroicons
==> pow
==> pow_assent
<bomb>

I do agree with the idea of flipping the boolean to favor the people more up to date, but this would just pass the buck to the older folks :)

danschultzer commented 1 year ago

Yeah, that looks right. So maybe it's the vsn lookup that fails. Can you try add this to deps/pow_assent/[lib/pow_assent/phoenix/controllers/registration_html.ex before the Pow.dependency_vsn_match?(:phoenix, "< 1.7.0") line:

  dbg :application.get_key(:phoenix, :vsn)
  dbg Pow.dependency_vsn_match?(:phoenix, ">= 1.7.0")

  # before this:
  if Pow.dependency_vsn_match?(:phoenix, ">= 1.7.0") do

Clear _build and build again. It would also be useful to know what system you run this on (with OTP and Elixir versions) in both local and CI, but I doubt it has anything to do with this.

danschultzer commented 1 year ago

After reading through erlang/elixir docs I suspect it'll return :undefined which means the application is not loaded. I haven't experienced this issue before, but maybe that's how it works compile-time and there's no guarantee that the Phoenix application is loaded. The solution would be to force load it on compile-time, or guessing the Phoenix version with some module.

I'll work on a PR that solves this. I hope to find a simple way to fetch the version from the project lock manifest, so there's no dependency at all.

coladarci commented 1 year ago

Sounds good - I'll try out the debugging as well. Happy to try out a branch if you want to send one my way.

coladarci commented 1 year ago

FYI when I reverse the if/else, I get this

== Compilation error in file lib/pow_assent/phoenix/controllers/registration_html.ex ==
** (CompileError) nofile:8: undefined function __user_id_field__/2 (there is no such import)
    (elixir 1.14.0) expanding macro: Kernel.to_string/1
    nofile:8: (file)
    (pow 1.0.27) expanding macro: Pow.Phoenix.Template.template/3
    lib/pow_assent/phoenix/controllers/registration_html.ex:19: PowAssent.Phoenix.RegistrationHTML (module)
    (elixir 1.14.0) expanding macro: Kernel.if/2
    lib/pow_assent/phoenix/controllers/registration_html.ex:8: PowAssent.Phoenix.RegistrationHTML (module)

Might be a clue...

danschultzer commented 1 year ago

I think it's just the same issue in Pow (since it's also using Pow.dependency_vsn_match?(:phoenix, ">= 1.7.0")):

https://github.com/pow-auth/pow/blob/0797fbe62496a63bfa3a8657c5d02c3fdb569182/lib/pow/phoenix/template.ex#L23-L35

        if Pow.dependency_vsn_match?(:phoenix, ">= 1.7.0") do
          quote do
            use Phoenix.Component
            import Pow.Phoenix.HTML.CoreComponents
            alias Phoenix.LiveView.JS
          end
        else
          # TODO: Remove when Phoenix 1.7 is required
          quote do
            import Pow.Phoenix.HTML.ErrorHelpers, only: [error_tag: 2]
            import Phoenix.HTML.{Form, Link}
          end
        end
danschultzer commented 1 year ago

Ok, try set Pow with this branch and see if it compiles correctly:

{:pow, git: "https://github.com/pow-auth/pow.git", branch: "compile-time-deps", override: true}

I'm loading all the deps at compile-time in that branch to get the version.

danschultzer commented 1 year ago

Sorry, pushed too early and didn't test properly. I've rebased and pushed the correct version. It should be good to go now. Lmk if this works.

coladarci commented 1 year ago

Oh my goodness. sooooo in doing this look what I found in my mix.exs file...

      # https://github.com/danschultzer/pow/issues/681
      {:pow, github: "danschultzer/pow", branch: "master", override: true},

what are the chances you moved this repo and I'm getting dead old master and it's causing problems?

danschultzer commented 1 year ago

Oh interesting. I believe Github does redirect essentially forever when repos are moved. But the branch name has been changed to main! Not sure if it causes any issues since that code haven't changed in a long time. But it would still be worth a try to pull latest main and see if there's something weird going on there.

coladarci commented 1 year ago

ok, so I put my mix file back to {:pow, "~> 1.0"}, and CI + Local now compiles.

I have a bunch of test failures I'll look into now, all similar to this:

Error:      test/scorpion_web/controllers/user_management_test.exs:28
     ** (ArgumentError) no "new" html template defined for ScorpionWeb.Pow.RegistrationHTML
     code: assert conn |> get(path) |> html_response(200) =~ "Sign Up"
     stacktrace:
       (phoenix_template 1.0.1) lib/phoenix/template.ex:241: Phoenix.Template.render_with_fallback/4
       (phoenix_template 1.0.1) lib/phoenix/template.ex:197: Phoenix.Template.render_within_layout/4
       (phoenix_template 1.0.1) lib/phoenix/template.ex:126: Phoenix.Template.render_to_iodata/4
       (phoenix 1.7.2) lib/phoenix/controller.ex:1010: anonymous fn/5 in Phoenix.Controller.template_render_to_iodata/4
       (telemetry 1.2.1) /runner/_work/scorpion/scorpion/deps/telemetry/src/telemetry.erl:321: :telemetry.span/3
       (phoenix 1.7.2) lib/phoenix/controller.ex:976: Phoenix.Controller.render_and_send/4
       (pow 1.0.29) lib/pow/phoenix/controllers/registration_controller.ex:1: Pow.Phoenix.RegistrationController.action/2
       (pow 1.0.29) lib/pow/phoenix/controllers/registration_controller.ex:1: Pow.Phoenix.RegistrationController.phoenix_controller_pipeline/2
       (phoenix 1.7.2) lib/phoenix/router.ex:430: Phoenix.Router.__call__/5
       (scorpion 0.1.0) lib/scorpion_web/endpoint.ex:1: ScorpionWeb.Endpoint.plug_builder_call/2
       (scorpion 0.1.0) lib/scorpion_web/endpoint.ex:1: ScorpionWeb.Endpoint."call (overridable 3)"/2
       (scorpion 0.1.0) lib/scorpion_web/endpoint.ex:1: ScorpionWeb.Endpoint.call/2
       (phoenix 1.7.2) lib/phoenix/test/conn_test.ex:225: Phoenix.ConnTest.dispatch/5
       test/scorpion_web/controllers/user_management_test.exs:30: (test)
danschultzer commented 1 year ago

Great! Sounds like git source changes the compilation tree.

Those errors seems to be the same dependency issue though, and it still can't fetch the version for Phoenix for whatever reason (which I suspect because the app is not loaded at that point).

coladarci commented 1 year ago

Got it - I just flipped my PR to use {:pow, git: "https://github.com/pow-auth/pow.git", branch: "compile-time-deps", override: true} and I'm getting the same errors

danschultzer commented 1 year ago

Ok, alternative solution. Try this:

{:pow, git: "https://github.com/pow-auth/pow.git", branch: "load-app", override: true}

coladarci commented 1 year ago

Same test failures, an example:

** (ArgumentError) no "new" html template defined for ScorpionWeb.PowResetPassword.ResetPasswordHTML
     code: assert conn |> get(path) |> html_response(200) =~ "Reset Password"
     stacktrace:
       (phoenix_template 1.0.1) lib/phoenix/template.ex:241: Phoenix.Template.render_with_fallback/4
       (phoenix_template 1.0.1) lib/phoenix/template.ex:197: Phoenix.Template.render_within_layout/4
       (phoenix_template 1.0.1) lib/phoenix/template.ex:126: Phoenix.Template.render_to_iodata/4
       (phoenix 1.7.2) lib/phoenix/controller.ex:1010: anonymous fn/5 in Phoenix.Controller.template_render_to_iodata/4
       (telemetry 1.2.1) /Users/greg/Code/scorpion/deps/telemetry/src/telemetry.erl:321: :telemetry.span/3
       (phoenix 1.7.2) lib/phoenix/controller.ex:976: Phoenix.Controller.render_and_send/4
       (pow 1.0.29) lib/extensions/reset_password/phoenix/controllers/reset_password_controller.ex:1: PowResetPassword.Phoenix.ResetPasswordController.action/2
       (pow 1.0.29) lib/extensions/reset_password/phoenix/controllers/reset_password_controller.ex:1: PowResetPassword.Phoenix.ResetPasswordController.phoenix_controller_pipeline/2
       (phoenix 1.7.2) lib/phoenix/router.ex:430: Phoenix.Router.__call__/5
       (scorpion 0.1.0) lib/scorpion_web/endpoint.ex:1: ScorpionWeb.Endpoint.plug_builder_call/2
       (scorpion 0.1.0) lib/scorpion_web/endpoint.ex:1: ScorpionWeb.Endpoint."call (overridable 3)"/2
       (scorpion 0.1.0) lib/scorpion_web/endpoint.ex:1: ScorpionWeb.Endpoint.call/2
       (phoenix 1.7.2) lib/phoenix/test/conn_test.ex:225: Phoenix.ConnTest.dispatch/5
       test/scorpion_web/controllers/user_management_test.exs:23: (test)
coladarci commented 1 year ago

I should note these are "real" errors; in that they also happen when running my app, not just in CI/Tests image

danschultzer commented 1 year ago

Hmm, I'm unable to trigger this. I could get to a state where it failed compile-time, but everything is good now on a Phoenix 1.7.2 installation.

Can you share what's in your mix project config? For reference this is what I'm testing with:

  def project do
    [
      app: :pow_test_17,
      version: "0.1.0",
      elixir: "~> 1.14",
      elixirc_paths: elixirc_paths(Mix.env()),
      start_permanent: Mix.env() == :prod,
      aliases: aliases(),
      deps: deps()
    ]
  end

Also application might show something:

  def application do
    [
      mod: {PowTest17.Application, []},
      extra_applications: [:logger, :runtime_tools]
    ]
  end
danschultzer commented 1 year ago

Maybe this can give us a clue. If you add dbg Application.loaded_applications and dbg Application.spec to deps/pow/lib/pow.ex:

  def dependency_vsn_match?(dep, req) do
    Application.load(dep)

    dbg Application.loaded_applications()
    dbg Application.spec(dep)

    case Application.spec(dep, :vsn) do
      nil -> false
      vsn -> Version.match?(List.to_string(vsn), req)
    end
  end

It should print out something in the console both during compile and also when running the app visiting sign in.

coladarci commented 1 year ago

At compile-time I see this:

[lib/pow.ex:12: Pow.dependency_vsn_match?/2]
Application.spec(dep) #=> [
  description: 'Peace of mind from prototype to production',
  id: [],
  vsn: '1.7.2',
  modules: [Mix.Phoenix, Mix.Phoenix.Context, Mix.Phoenix.Schema,
   Mix.Tasks.Compile.Phoenix, Mix.Tasks.Phx, Mix.Tasks.Phx.Digest,
   Mix.Tasks.Phx.Digest.Clean, Mix.Tasks.Phx.Gen, Mix.Tasks.Phx.Gen.Auth,
   Mix.Tasks.Phx.Gen.Auth.HashingLibrary, Mix.Tasks.Phx.Gen.Auth.Injector,
   Mix.Tasks.Phx.Gen.Auth.Migration, Mix.Tasks.Phx.Gen.Cert,
   Mix.Tasks.Phx.Gen.Channel, Mix.Tasks.Phx.Gen.Context,
   Mix.Tasks.Phx.Gen.Embedded, Mix.Tasks.Phx.Gen.Html, Mix.Tasks.Phx.Gen.Json,
   Mix.Tasks.Phx.Gen.Live, Mix.Tasks.Phx.Gen.Notifier,
   Mix.Tasks.Phx.Gen.Presence, Mix.Tasks.Phx.Gen.Release,
   Mix.Tasks.Phx.Gen.Schema, Mix.Tasks.Phx.Gen.Secret, Mix.Tasks.Phx.Gen.Socket,
   Mix.Tasks.Phx.Routes, Mix.Tasks.Phx.Server, Phoenix,
   Phoenix.ActionClauseError, Phoenix.Channel, Phoenix.Channel.Server,
   Phoenix.ChannelTest, Phoenix.ChannelTest.NoopSerializer,
   Phoenix.CodeReloader, Phoenix.CodeReloader.Proxy,
   Phoenix.CodeReloader.Server, Phoenix.Config, Phoenix.ConnTest,
   Phoenix.Controller, Phoenix.Controller.Pipeline, Phoenix.Digester,
   Phoenix.Digester.Compressor, Phoenix.Digester.Gzip, Phoenix.Endpoint,
   Phoenix.Endpoint.Cowboy2Adapter, Phoenix.Endpoint.RenderErrors, ...],
  maxP: :infinity,
  maxT: :infinity,
  registered: [],
  included_applications: [],
  optional_applications: [],
  applications: [:kernel, :stdlib, :elixir, :logger, :eex, :crypto, :public_key,
   :plug, :plug_crypto, :telemetry, :phoenix_pubsub, :phoenix_template,
   :websock_adapter, :castore],
  mod: {Phoenix, []},
  start_phases: :undefined
]

At runtime I get a ton of Pry requests

Request to pry #PID<0.16363.0> at Pow.dependency_vsn_match?/2 (lib/pow.ex:11)

    8:   def dependency_vsn_match?(dep, req) do
    9:     Application.load(dep)
   10: 
   11:     dbg(Application.loaded_applications())
   12:     dbg(Application.spec(dep))
   13: 
   14:     case Application.spec(dep, :vsn) do

But when it eventually settles, I get the same 500 with no new debugging; I assume the necessary info is at compile time.

In regards to the app config

def project do
    [
      app: :scorpion,
      version: "0.1.0",
      elixir: "~> 1.14",
      elixirc_paths: elixirc_paths(Mix.env()),
      start_permanent: Mix.env() == :prod,
      test_coverage: [tool: ExCoveralls],
      preferred_cli_env: [
        coveralls: :test,
        "coveralls.detail": :test,
        "coveralls.post": :test,
        "coveralls.html": :test
      ],
      dialyzer: [
        plt_add_deps: :app_tree,
        flags: [:error_handling],
        ignore_warnings: ".dialyzer_ignore.exs",
        plt_core_path: "_build/#{Mix.env()}"
      ],
      aliases: aliases(),
      deps: deps()
    ]
  end

+

  def application do
    [
      mod: {Scorpion.Application, []},
      extra_applications: [:logger, :runtime_tools]
    ]
  end

I'm sorry none of this is probably of any use..

danschultzer commented 1 year ago

It seems to work correctly. Can you send me what's in ScorpionWeb.Pow.SessionHTML?

coladarci commented 1 year ago

ScorpionWeb.Pow.SessionHTML isn't defined in my app - what specifically am I looking for and where?

danschultzer commented 1 year ago

Ah, I think that's the issue. You have upgraded to 1.7 and used Pow with custom templates in 1.6?

You will need to update ScorpionWeb.Pow.SessionView to ScorpionWeb.Pow.SessionHTML. The module file has also changed to use embed_templates. I recommend running mix pow.phoenix.gen.templates to get the up-to-date Phoenix 1.7 format.

danschultzer commented 1 year ago

I've also merged the changes so you should just pull from main now:

{:pow, git: "https://github.com/pow-auth/pow.git", branch: "main", override: true}

coladarci commented 1 year ago

Ah, I think that's the issue. You have upgraded to 1.7 and used Pow with custom templates in 1.6?

Ah, ok, yeah..

So we make heavy use of custom templates so this will take some work. We have a situation where we are using both live-view and non-live-view within our app (we couldn't just flip the entire thing over). SO, our login flow is not using live view currently but maybe with this update it'll be easier to do so..

Feel free to close this or keep it open to track the progress but at this point I think we got to the bottom of it!

Appreciate all your help; sounds like a good bug fix was made and I have some upgrade drama ahead of me :)

danschultzer commented 1 year ago

Nice, I've released Pow v1.0.30 so you can just go back to using:

{:pow, "~> 1.0.30"}

Also here's a quick guide for upgrade Pow templates:

  1. Move views from views/pow/ to controllers/pow/ and rename them from NAME_view.ex to NAME_html.ex
  2. Move templates from templates/pow/NAME/ to controllers/pow/NAME_html/
  3. Change the module from:

    defmodule MyAppWeb.Pow.NAMEView do
      @moduledoc false
      use MyAppWeb, :view
    end

    to:

    defmodule MyAppWeb.Pow.NAMEHTML do
      @moduledoc false
      use MyAppWeb, :html
    
      embed_templates "NAME_html/*"
    end

NAME being replaced with the view name, e.g. SessionView -> SessionHTML and templates/pow/session/new.html.eex -> controllers/pow/session_html/new.html.eex

coladarci commented 1 year ago

What's crazy is when I do a simple regeneration of the templates, things load but it's loading the templates without a layout.. (UPDATE, I'm onto something.. will report back)

coladarci commented 1 year ago

Ok so am I correct that pow currently doesn't support live routes? i.e live "/sign_up" ___ v get "/sign_up", RegistrationController, :new ?

My global template, in the live-view world, have live components so the templates are failing to render the pow controller actions. I know I can get this to work by making some new layouts, etc, but it's a little temporary given many phoenix apps are moving to live routes instead of traditional controllers.

danschultzer commented 1 year ago

Ok so am I correct that pow currently doesn't support live routes?

Yeah, unfortunately there's no live route support. It hooks in with lack of WebSocket support in Pow. I hope to get it implemented soon though, but for now it's just plain routes and controllers.

danschultzer commented 1 year ago

But it does seems there's a way to use live component in dead view (depending how it has been set up): https://elixirforum.com/t/how-to-embed-liveview-component-in-deadview-regular-view/42971/3?u=danschultzer