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

💕 support for inline toml dictionaries #150

Merged
merged 19 commits into from
May 10, 2022

Conversation

cratelyn
Copy link
Contributor

@cratelyn cratelyn commented Apr 12, 2022

Fixes #142.

🤞 should also resolve #146.

this PR implements support for inline TOML dictionaries. today, dictionaries can be configured using JSON files stored on disk. as #142 outlines, this is a smidge inconsistent with the way that we configure backends. as discussion in #146 points out, this means that running a Wasm program that accesses a dictionary entails placing files on disk.

this new inline feature allows for dictionaries to be specified inline within the fastly.toml file itself. here is an example configuration pulled from the test coverage in this branch:

name = "inline-toml-dictionary-lookup"
description = "inline toml dictionary lookup test"
authors = ["Jill Bryson <[email protected]>", "Rose McDowall <[email protected]>"]
language = "rust"

[local_server]
[local_server.dictionaries]
[local_server.dictionaries.animals]
format = "inline-toml"
[local_server.dictionaries.animals.contents]
dog = "woof"
cat = "meow"

as a pleasant side-effect of this work, we are also able to refactor the dictionary deserialization and lookup logic so that we only read an external JSON file once.

@cratelyn cratelyn marked this pull request as draft April 12, 2022 22:30
@cratelyn cratelyn changed the title inline toml dictionaries 💕 support for inline toml dictionaries Apr 12, 2022
@cratelyn cratelyn self-assigned this Apr 12, 2022
@cratelyn cratelyn added the feature-ux Concerning ergonomics and ease-of-use label Apr 12, 2022
@cratelyn cratelyn force-pushed the katie/inline-toml-dictionaries branch from 0364738 to f4beefa Compare April 13, 2022 17:24
@cratelyn cratelyn marked this pull request as ready for review April 13, 2022 17:36
@cratelyn
Copy link
Contributor Author

cc: @kailan @triblondon i would love an eye from your team on this, particularly in regards to the extensions to fastly.toml here. 🙂

@cratelyn cratelyn requested a review from aturon April 13, 2022 17:38
@kailan
Copy link
Member

kailan commented Apr 13, 2022

Thanks for the ping @cratelyn !!

This looks fantastic. I only have a small nit for the sake of consistency: rather than contents I think it would be better to use the term items here as that is the terminology we use in the [setup] section and the API.

aturon
aturon previously approved these changes Apr 13, 2022
Copy link
Contributor

@aturon aturon left a comment

Choose a reason for hiding this comment

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

Fantastic work @cratelyn! Love the tests and the general improvements you made along the way 🎉

lib/src/wiggle_abi/dictionary_impl.rs Outdated Show resolved Hide resolved
@ccampbell
Copy link

ccampbell commented Apr 13, 2022

Just wanted to leave a quick comment related to this:

as a pleasant side-effect of this work, we are also able to refactor the dictionary deserialization and lookup logic so that we only read an external JSON file once.

In production, dictionary updates are live, so I actually depend on that functionality during development (being able to update JSON dictionaries and see the updates reflected instantly on the next request without having to restart the server). Just want to make sure we don’t remove that functionality as a result of these updates.

Perhaps it was more of an unintended consequence of the previous implementation vs. by design, but it would be nice if we could preserve that behavior (even with the new format?!)

@cratelyn
Copy link
Contributor Author

cratelyn commented May 6, 2022

Just wanted to leave a quick comment related to this:

as a pleasant side-effect of this work, we are also able to refactor the dictionary deserialization and lookup logic so that we only read an external JSON file once.

In production, dictionary updates are live, so I actually depend on that functionality during development (being able to update JSON dictionaries and see the updates reflected instantly on the next request without having to restart the server). Just want to make sure we don’t remove that functionality as a result of these updates.

Perhaps it was more of an unintended consequence of the previous implementation vs. by design, but it would be nice if we could preserve that behavior (even with the new format?!)

this is done in 44e4bf6! thank you for raising this point, i agree that this is a nice behavior to maintain. 🙂

katelyn martin added 2 commits May 6, 2022 14:25
this is a change to the payload(s) of our dictionary config error
variants, specifically removing dictionary names from these variants.

this is a precursor to sharing logic to read the contents of a JSON
dictionary both at startup, and later when a dictionary lookup is
performed.

at startup, these errors are propagated up into a variant that already
includes the name:

```rust
    #[error("invalid configuration for '{name}': {err}")]
    InvalidDictionaryDefinition {
        name: String,
        #[source]
        err: DictionaryConfigError,
    },
```

later on, we can similarly deduce the name of the dictionary given a
handle.
this modifies our dictionary logic so that we read external files every
time that a lookup is performed. this is helpful so that people do not
need to restart the server every time they change a dictionary file.
@cratelyn cratelyn force-pushed the katie/inline-toml-dictionaries branch from 61948f3 to 44e4bf6 Compare May 6, 2022 18:25
@cratelyn cratelyn requested a review from aturon May 6, 2022 20:05
Copy link
Contributor

@aturon aturon left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for making the live-update adjustment 👍

@cratelyn cratelyn merged commit 4efba13 into main May 10, 2022
@cratelyn cratelyn deleted the katie/inline-toml-dictionaries branch May 10, 2022 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-ux Concerning ergonomics and ease-of-use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make environment variables available inside local server Support TOML inline dictionaries
4 participants