Asset Configuration #239

xn · 2023-05-15T15:09:57Z

Adds config for assets allowed, backends and validations.

Whitelist specific mime types. `["image/gif", "image/png", "image.jpeg"]
Assign multiple backends to mime type or catchall: "image/jpeg" or "image/*"

leandrocp

Hey @xn some comments before we can move forward. Thanks

leandrocp · 2023-05-15T15:24:26Z

lib/beacon_web/live/admin/media_library_live/upload_form_component.ex

+    sites = Beacon.Registry.registered_sites()
+    Enum.map(sites, fn site -> to_string(site) end)


I think you can let phoenix convert to string:

Suggested change

sites = Beacon.Registry.registered_sites()

Enum.map(sites, fn site -> to_string(site) end)

Beacon.Registry.registered_sites()

leandrocp · 2023-05-15T15:26:26Z

lib/beacon/admin/media_library/asset.ex

+    |> send_to_provider(metadata)
+  end
+
+  def send_to_provider(asset, metadata) do


Suggested change

def send_to_provider(asset, metadata) do

defp send_to_provider(asset, metadata) do

leandrocp · 2023-05-15T15:32:30Z

lib/beacon/admin/media_library/provider/repo.ex

@@ -0,0 +1,16 @@
+defmodule Beacon.Admin.MediaLibrary.Provider.Repo do


The config is list(backend :: module()). Perhaps we should rename to Backend.Repo or rename :backends to :providers?

Backend.Repo is just fine.

leandrocp · 2023-05-15T15:45:08Z

dev.exs

-    "dockyard_2.jpg",
-    "image/jpg"
-  )
+  metadata =


what if we make building this struct private by moving FileMetadata.new/5 to inside def upload/1? the caller doesn't need to know about it and it's less risky.

The caller would just Beacon developers, right?

So my thinking for doing this is developer experience

Lower cognitive overhead. It's easier to reason about a well defined struct, than a 5 param function (which order?) and still have to keep the idea of the struct anyway for working on the internals.

The growth of the function signature of upload. 5 params in upload is already a bit much. Adding any opts to this turns it into a mess to think about. vid, What's an option vs parameter, etc.

Testing. I can now create well named Metadata factories for reuse.

I see the benefits for testing and I agree that 5+ args is too much (it has 4 now). I'm just worried that we're exposing FileMetadata internals that should be private. One could write upload(%FileMetadata{config: nil}) and break the pipeline, which I see is possible since the spec would look like @spec upload(FileMetadata.t), right?

Can you please add a TODO: https://github.com/BeaconCMS/beacon/pull/239#discussion_r1194160478 close to def new? To serve as a reminder to revisit it before v0.1 so we can move forward with this PR.

Will do!

A developer could write upload(%FileMetadata{config: nil}) without a catchall, but it would have to go through a PR process.

If you simply prefer to have the FileMetadata constructed inside the function, that is fine and I'm happy to change it.

Doing that though means either upload needs to accept a file size, or we need to calculate it somewhere.

Let's keep it as it's and see how it plays as we work on Admin

leandrocp · 2023-05-15T15:59:57Z

lib/beacon/config.ex

    Registry.config!(site)
  end
+
+  @spec config_for_mime_type(Beacon.Config.t(), MIME.t()) :: term()
+  def config_for_mime_type(beacon_config, mime_type) do


If this fn can raise it's better to be append !

Suggested change

def config_for_mime_type(beacon_config, mime_type) do

def config_for_mime_type!(beacon_config, mime_type) do

But it should be better to return a Beacon error with a friendly message instead of KeyError.

leandrocp · 2023-05-15T16:01:40Z

lib/beacon/config.ex

    Registry.config!(site)
  end
+
+  @spec config_for_mime_type(Beacon.Config.t(), MIME.t()) :: term()


you could move Enum.into(%{}) into config_for_mime_type/2

Suggested change

@spec config_for_mime_type(Beacon.Config.t(), MIME.t()) :: term()

@spec config_for_mime_type(Beacon.Config.t(), MIME.t()) :: map()

I guess the question here is, this function currently returns data of a type encoded by this module and do we want to return something else? I think when I originally wrote this, I hadn't created the mime_config type.

So it should be @spec config_for_mime_type(Beacon.Config.t(), MIME.t()) :: mime_config()
unless you really want to return a map there.

unless you really want to return a map there.

Nope. It was just to avoid forcing the caller to transform the returned data.

leandrocp · 2023-05-15T16:02:32Z

lib/beacon/config.ex

@@ -48,6 +48,14 @@ defmodule Beacon.Config do
  """
  @type template_formats :: [{format :: atom(), description :: String.t()}]

+  @type asset :: [{MIME.t(), mime_config()}]


I don't think MIME.t/0 exists, it's just a String.t/0 :)
but we can have our own custom type

You're right. I meant to ask if you wanted anything special there and that was a place keeper.

String.t/0 or a custom ~~ContentType.t()~~ MediaType.t/0 works for me, whatever you prefer 👍🏻

leandrocp · 2023-05-15T16:21:14Z

lib/beacon/admin/media_library/file_metadata.ex

+      |> Beacon.Config.config_for_mime_type(mime_type)
+      |> Enum.into(%{})
+
+    %__MODULE__{


I see some inconsistency on the "type" key:

config.asset_formats

metadata.allowed_types

metadata.mime_type

asset.file_type

The term "MIME type" was updated to "Media Type" as noted at https://www.iana.org/assignments/media-types/media-types.xhtml so we have 3 acceptable suggestions:

media_type - technically the correct name

mime_type - still popular

type

I'm more inclined to suggest calling it just type because image/* isn't a registered and valid media type and IANA calls the main categoryimage as "type", see https://www.iana.org/assignments/media-types/image/webp - that would look like:

config.asset_types

metadata.allowed_types

metadata.type

asset.type

Wdyt?

You are correct on multiple accounts here.

* is not a valid mime sub type

naming variance.

image/* is a valid content type, however.

I'm comfortable with your name changes. Does content_type hold anything for you? It can point the way on what is expected there.

image/* is a valid content type, however

that's a good point, I'm okay with using content_type

I was also glomming onto what Plug.Conn was doing:

https://hexdocs.pm/plug/Plug.Conn.Utils.html#media_type/1

Sorry for the confusion but giving a second thought, I think it's better to call it media_type for the following reasons:

type is too generic

media_type is technically correct

content_type serves to another purpose

having image/* as the media type is fine since content type and Plug accepts it

Sounds good?

works for me!

xn · 2023-05-15T18:39:25Z

Thanks for the great feedback, @leandrocp !

leandrocp

Nice one

xn requested a review from leandrocp May 15, 2023 15:10

xn marked this pull request as draft May 15, 2023 15:10

leandrocp reviewed May 15, 2023

View reviewed changes

xn force-pushed the asset_provider branch from c67581d to 2ca9844 Compare May 16, 2023 11:07

Asset Configuration

b366ffb

xn force-pushed the asset_provider branch from 2ca9844 to b366ffb Compare May 16, 2023 18:32

xn marked this pull request as ready for review May 16, 2023 18:32

xn added 2 commits May 16, 2023 15:03

cleanup types

cd5fecc

fix typo

2931f0b

leandrocp approved these changes May 16, 2023

View reviewed changes

xn merged commit 4e06457 into main May 16, 2023
3 checks passed

xn deleted the asset_provider branch May 16, 2023 19:29

leandrocp mentioned this pull request Nov 1, 2023

Core -> CDN Integration: CDN configuration #221

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asset Configuration #239

Asset Configuration #239

xn commented May 15, 2023

leandrocp left a comment

leandrocp May 15, 2023

leandrocp May 15, 2023

leandrocp May 15, 2023

xn May 15, 2023

leandrocp May 15, 2023

xn May 15, 2023

leandrocp May 15, 2023

leandrocp May 15, 2023

xn May 15, 2023

leandrocp May 15, 2023

leandrocp May 15, 2023

xn May 15, 2023

leandrocp May 15, 2023

xn May 15, 2023

leandrocp May 15, 2023

leandrocp May 15, 2023

xn May 15, 2023

leandrocp May 15, 2023 •

edited

leandrocp May 15, 2023

xn May 15, 2023

leandrocp May 15, 2023

xn May 15, 2023

leandrocp May 15, 2023

xn May 15, 2023

xn commented May 15, 2023

leandrocp left a comment

		sites = Beacon.Registry.registered_sites()
		Enum.map(sites, fn site -> to_string(site) end)

	sites = Beacon.Registry.registered_sites()
	Enum.map(sites, fn site -> to_string(site) end)
	Beacon.Registry.registered_sites()

	def send_to_provider(asset, metadata) do
	defp send_to_provider(asset, metadata) do

		@@ -0,0 +1,16 @@
		defmodule Beacon.Admin.MediaLibrary.Provider.Repo do

	def config_for_mime_type(beacon_config, mime_type) do
	def config_for_mime_type!(beacon_config, mime_type) do

	@spec config_for_mime_type(Beacon.Config.t(), MIME.t()) :: term()
	@spec config_for_mime_type(Beacon.Config.t(), MIME.t()) :: map()

Asset Configuration #239

Asset Configuration #239

Conversation

xn commented May 15, 2023

leandrocp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leandrocp May 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xn commented May 15, 2023

leandrocp left a comment

Choose a reason for hiding this comment

leandrocp May 15, 2023 •

edited