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.

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

Implement file management #2022

Merged
merged 11 commits into from
Jul 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions assets/css/js_interop.css
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,11 @@ solely client-side operations.
@apply hidden;
}

[data-el-session]:not([data-js-side-panel-content="files-list"])
[data-el-files-list] {
@apply hidden;
}

[data-el-session]:not([data-js-side-panel-content="runtime-info"])
[data-el-runtime-info] {
@apply hidden;
Expand All @@ -249,6 +254,11 @@ solely client-side operations.
@apply text-gray-50 bg-gray-700;
}

[data-el-session][data-js-side-panel-content="files-list"]
[data-el-files-list-toggle] {
@apply text-gray-50 bg-gray-700;
}

[data-el-session][data-js-side-panel-content="runtime-info"]
[data-el-runtime-info-toggle] {
@apply text-gray-50 bg-gray-700;
Expand Down
10 changes: 10 additions & 0 deletions assets/js/hooks/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ const Session = {
this.toggleAppInfo()
);

this.getElement("files-list-toggle").addEventListener("click", (event) =>
this.toggleFilesList()
);

this.getElement("notebook").addEventListener("scroll", (event) =>
this.updateSectionListHighlight()
);
Expand Down Expand Up @@ -368,6 +372,8 @@ const Session = {
this.toggleAppInfo();
} else if (keyBuffer.tryMatch(["s", "u"])) {
this.toggleClientsList();
} else if (keyBuffer.tryMatch(["s", "f"])) {
this.toggleFilesList();
} else if (keyBuffer.tryMatch(["s", "r"])) {
this.toggleRuntimeInfo();
} else if (keyBuffer.tryMatch(["s", "b"])) {
Expand Down Expand Up @@ -719,6 +725,10 @@ const Session = {
this.toggleSidePanelContent("app-info");
},

toggleFilesList() {
this.toggleSidePanelContent("files-list");
},

toggleRuntimeInfo() {
this.toggleSidePanelContent("runtime-info");
},
Expand Down
12 changes: 7 additions & 5 deletions lib/livebook/file_system/s3.ex
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,11 @@ defmodule Livebook.FileSystem.S3 do
bucket_url = String.trim_trailing(bucket_url, "/")
region = opts[:region] || region_from_uri(bucket_url)

hash = :crypto.hash(:sha256, bucket_url) |> Base.url_encode64(padding: false)
id = "s3-#{hash}"
Comment on lines +33 to +34
Copy link
Member Author

@jonatanklosko jonatanklosko Jun 29, 2023

Choose a reason for hiding this comment

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

Currently we are using random ids generated when the file system is added, which is fine for settings (local). However, to persist a file link in a notebook, we need a deterministic file system id, so if multiple Livebook users use the same S3 bucket, the file link is exported/imported as expected.

I did the hash so that the id is fixed length and this way it also has a predictable character set. Also, even though buckets generally need authorisation, not listing the URL explicitly sounds like an advantage to me.

I considered a second field/function like :public_id, but having both random :id and the :public_id is confusing, especially that both should be unique anyway. Also, having a deterministic :id is what we do for hubs too, so it's better for consistency. To avoid breaking existing settings I added a migration with more details.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me. I also wonder if the ID should be the bucket itself but this at least avoids us from leaking bucket information.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I would still like base64 encode the bucket URL to avoid weird characters in the ID, but not leaking sounds preferable anyway.


%__MODULE__{
id: Livebook.Utils.random_short_id(),
id: id,
bucket_url: bucket_url,
region: region,
access_key_id: access_key_id,
Expand All @@ -52,23 +55,22 @@ defmodule Livebook.FileSystem.S3 do
def from_config(config) do
case config do
%{
id: id,
bucket_url: bucket_url,
access_key_id: access_key_id,
secret_access_key: secret_access_key
} ->
file_system = new(bucket_url, access_key_id, secret_access_key, region: config[:region])
{:ok, %{file_system | id: id}}
{:ok, file_system}

_config ->
{:error,
"S3 file system config is expected to have keys: :id, :bucket_url, :access_key_id and :secret_access_key, but got #{inspect(config)}"}
"S3 file system config is expected to have keys: :bucket_url, :access_key_id and :secret_access_key, but got #{inspect(config)}"}
end
end

@spec to_config(t()) :: map()
def to_config(%__MODULE__{} = s3) do
Map.take(s3, [:id, :bucket_url, :region, :access_key_id, :secret_access_key])
Map.take(s3, [:bucket_url, :region, :access_key_id, :secret_access_key])
end
end

Expand Down
40 changes: 30 additions & 10 deletions lib/livebook/live_markdown/export.ex
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,16 @@ defmodule Livebook.LiveMarkdown.Export do

app_settings_metadata = app_settings_metadata(notebook.app_settings)

if app_settings_metadata == %{} do
metadata
else
Map.put(metadata, :app_settings, app_settings_metadata)
end
file_entry_metadatas =
notebook.file_entries
|> Enum.sort_by(& &1.name)
|> Enum.map(&file_entry_metadata/1)

put_unless_default(
metadata,
%{app_settings: app_settings_metadata, file_entries: file_entry_metadatas},
%{app_settings: %{}, file_entries: []}
)
end

defp app_settings_metadata(app_settings) do
Expand All @@ -108,6 +113,18 @@ defmodule Livebook.LiveMarkdown.Export do
)
end

defp file_entry_metadata(%{type: :attachment, name: name}) do
%{type: "attachment", name: name}
end

defp file_entry_metadata(%{type: :file, name: name, file: file}) do
%{type: "file", name: name, file: %{file_system_id: file.file_system.id, path: file.path}}
end

defp file_entry_metadata(%{type: :url, name: name, url: url}) do
%{type: "url", name: name, url: url}
end

defp render_section(section, notebook, ctx) do
name = ["## ", section.name]

Expand Down Expand Up @@ -362,13 +379,16 @@ defmodule Livebook.LiveMarkdown.Export do
put_unless_default(%{}, Map.take(notebook, keys), Map.take(Notebook.new(), keys))
end

defp ensure_order(map) do
defp ensure_order(%{} = map) do
map
|> Enum.sort()
|> Enum.map(fn
{key, %{} = value} -> {key, ensure_order(value)}
pair -> pair
end)
|> Enum.map(fn {key, value} -> {key, ensure_order(value)} end)
|> Jason.OrderedObject.new()
end

defp ensure_order(list) when is_list(list) do
Enum.map(list, &ensure_order/1)
end

defp ensure_order(term), do: term
end
98 changes: 84 additions & 14 deletions lib/livebook/live_markdown/import.ex
Original file line number Diff line number Diff line change
Expand Up @@ -381,35 +381,58 @@ defmodule Livebook.LiveMarkdown.Import do

defp notebook_metadata_to_attrs(metadata) do
Enum.reduce(metadata, {%{}, Livebook.Hubs.Personal.id(), []}, fn
{"persist_outputs", persist_outputs}, {attrs, id, messages} ->
{Map.put(attrs, :persist_outputs, persist_outputs), id, messages}
{"persist_outputs", persist_outputs}, {attrs, stamp_hub_id, messages} ->
{Map.put(attrs, :persist_outputs, persist_outputs), stamp_hub_id, messages}

{"autosave_interval_s", autosave_interval_s}, {attrs, id, messages} ->
{Map.put(attrs, :autosave_interval_s, autosave_interval_s), id, messages}
{"autosave_interval_s", autosave_interval_s}, {attrs, stamp_hub_id, messages} ->
{Map.put(attrs, :autosave_interval_s, autosave_interval_s), stamp_hub_id, messages}

{"default_language", default_language}, {attrs, id, messages}
{"default_language", default_language}, {attrs, stamp_hub_id, messages}
when default_language in ["elixir", "erlang"] ->
default_language = String.to_atom(default_language)
{Map.put(attrs, :default_language, default_language), id, messages}
{Map.put(attrs, :default_language, default_language), stamp_hub_id, messages}

{"hub_id", hub_id}, {attrs, id, messages} ->
{"hub_id", hub_id}, {attrs, stamp_hub_id, messages} ->
cond do
Hubs.hub_exists?(hub_id) -> {Map.put(attrs, :hub_id, hub_id), hub_id, messages}
Hubs.get_offline_hub(hub_id) -> {attrs, hub_id, messages}
true -> {attrs, id, messages ++ ["ignoring notebook Hub with unknown id"]}
true -> {attrs, stamp_hub_id, messages ++ ["ignoring notebook Hub with unknown id"]}
end

{"app_settings", app_settings_metadata}, {attrs, id, messages} ->
{"app_settings", app_settings_metadata}, {attrs, stamp_hub_id, messages} ->
app_settings =
Map.merge(
Notebook.AppSettings.new(),
app_settings_metadata_to_attrs(app_settings_metadata)
)

{Map.put(attrs, :app_settings, app_settings), id, messages}

_entry, {attrs, id, messages} ->
{attrs, id, messages}
{Map.put(attrs, :app_settings, app_settings), stamp_hub_id, messages}

{"file_entries", file_entry_metadatas}, {attrs, stamp_hub_id, messages}
when is_list(file_entry_metadatas) ->
file_system_by_id =
if Enum.any?(file_entry_metadatas, &(&1["type"] == "file")) do
for file_system <- Livebook.Settings.file_systems(),
do: {file_system.id, file_system},
into: %{}
else
%{}
end

{file_entries, file_entry_messages} =
for file_entry_metadata <- file_entry_metadatas, reduce: {[], []} do
{file_entries, warnings} ->
case file_entry_metadata_to_attrs(file_entry_metadata, file_system_by_id) do
{:ok, file_entry} -> {[file_entry | file_entries], warnings}
{:error, message} -> {file_entries, [message | warnings]}
end
end

{Map.put(attrs, :file_entries, file_entries), stamp_hub_id,
messages ++ file_entry_messages}

_entry, {attrs, stamp_hub_id, messages} ->
{attrs, stamp_hub_id, messages}
end)
end

Expand Down Expand Up @@ -444,6 +467,33 @@ defmodule Livebook.LiveMarkdown.Import do
end)
end

defp file_entry_metadata_to_attrs(%{"type" => "attachment", "name" => name}, _file_system_by_id) do
{:ok, %{type: :attachment, name: name}}
end

defp file_entry_metadata_to_attrs(
%{
"type" => "file",
"name" => name,
"file" => %{"file_system_id" => file_system_id, "path" => path}
},
file_system_by_id
) do
if file_system = file_system_by_id[file_system_id] do
file = Livebook.FileSystem.File.new(file_system, path)
{:ok, %{type: :file, name: name, file: file}}
else
{:error, "skipping file #{name}, since it points to an unknown file system"}
end
end

defp file_entry_metadata_to_attrs(
%{"type" => "url", "name" => name, "url" => url},
_file_system_by_id
) do
{:ok, %{type: :url, name: name, url: url}}
end

defp section_metadata_to_attrs(metadata) do
Enum.reduce(metadata, %{}, fn
{"branch_parent_index", parent_idx}, attrs ->
Expand Down Expand Up @@ -518,7 +568,27 @@ defmodule Livebook.LiveMarkdown.Import do
end
end)

{%{notebook | sections: sections}, Enum.reverse(warnings)}
notebook = %{notebook | sections: sections}

legacy_images? =
notebook
|> Notebook.cells_with_section()
|> Enum.any?(fn {cell, _section} ->
# A heuristic to detect legacy image source
is_struct(cell, Notebook.Cell.Markdown) and String.contains?(cell.source, "](images/")
end)

image_warnings =
if legacy_images? do
[
"found Markdown images pointing to the images/ directory." <>
" Using this directory has been deprecated, please use notebook files instead"
]
else
[]
end

{notebook, Enum.reverse(warnings) ++ image_warnings}
end

defp take_stamp_data([{:stamp, data} | elements]), do: {data, elements}
Expand Down
41 changes: 41 additions & 0 deletions lib/livebook/migration.ex
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ defmodule Livebook.Migration do
insert_personal_hub()
move_app_secrets_to_personal_hub()
add_personal_hub_secret_key()
update_file_systems_to_deterministic_ids()
end

defp delete_local_host_hub() do
Expand Down Expand Up @@ -46,4 +47,44 @@ defmodule Livebook.Migration do
Livebook.Storage.insert(:hubs, Livebook.Hubs.Personal.id(), secret_key: secret_key)
end
end

defp update_file_systems_to_deterministic_ids() do
# We changed S3 file system ids, such that they are deterministic
# for the same bucket, rather than random. We take this opportunity
# to rename the scope from :filesystem to :file_systems, which
# conveniently allows for easy check if there's anything to migrate.
# This migration can be removed in the future (at the cost of discarding
# very old file systems (which can be re-added).
jonatanklosko marked this conversation as resolved.
Show resolved Hide resolved
# TODO: remove on Livebook v0.12

case Livebook.Storage.all(:filesystem) do
[] ->
:ok

configs ->
id_mapping =
for config <- configs, into: %{} do
old_id = config.id
# At this point S3 is the only file system we store
{:ok, file_system} = Livebook.FileSystem.S3.from_config(config)
Livebook.Settings.save_file_system(file_system)
Livebook.Storage.delete(:filesystem, old_id)
{old_id, file_system.id}
end

# Remap default file system id

default_file_system_id = Livebook.Settings.default_file_system_id()

if new_id = id_mapping[default_file_system_id] do
Livebook.Settings.set_default_file_system(new_id)
end

# Livebook.NotebookManager is started before the migration runs,
# and it discards S3 files, since it can't find the file system.
# However, in this case it's fine; for recent notebooks it does
# not matter really and there are likely not many starred S3
# notebooks at this point (and the user can easily star again)
end
end
end
Loading
Loading