Add local dictionary support #61

JakeChampion · 2021-08-09T15:11:48Z

Resolves #11

This reads a new section from the local_server part of fastly.toml named dictionaries.

The local_server.dictionaries section is an array of tables with two fields: name, and file.

The name field is a string which is used as the name of the dictionary and is used via the Dictionary.open method. This field uses the same validation rules asthe Fastly API (Name must start with alphabetical and contain only alphanumeric, underscore, and whitespace).

The file field is a string which is a file-path pointing to a JSON file which contains all the dictionary items within an object.

An example of a full local_server section of fastly.toml now looks like this:

[local_server]
  [local_server.backends]
    [local_server.backends.dog]
      url = "http://localhost:7878/dog-mocks"
  [local_server.dictionaries]
    [local_server.dictionaries.a]
      file="./a.json"
      format = "json"

Note for reviewers:

I couldn't figure out how the tests are implements for the wiggle abi parts such as lib/src/wiggle_abi/dictionary_impl.rs. I assume they are implemented in an internal/private repo that Fastly has?

Reading dictionaries from fasty.toml file
- Note: I added the local dictionaries to the local_server section
Construct Dictionary and DictionaryHandles
- Dictionaries have two fields: name, and file
- Validate name field with Fastly' validation rules (Name must start with alphabetical and contain only alphanumeric, underscore, and whitespace)
- Validate file file (perhaps by doing a file existence check and checking it is JSON which adheres to our schema)
Implement Dictionary.open
Implement Dictionary.get
Tests
documentation

lib/src/wiggle_abi/dictionary_impl.rs

cratelyn

😻 This is incredible stuff! Thank you for opening this PR.

I have left assorted comments below; the two high-level points are that we should think about how we deserialize the configuration, and how we assign handles.

lib/src/error.rs

cli/src/main.rs

lib/src/config.rs

cli/src/main.rs

cratelyn · 2021-08-10T15:15:06Z

lib/src/config/unit_tests.rs

+        use DictionaryConfigError::UnrecognizedKey;
+        static BAD_DEFAULT: &str = r#"
+            [dictionaries]
+            thing = { name = "thing", file = "./file.json", shrimp = true }


ya love to see a real shrimp-:a:ck in the wild

lib/src/config/unit_tests.rs

lib/src/config/dictionaries.rs

lib/src/session.rs

JakeChampion · 2021-08-10T20:29:07Z

Thanks for the thorough review @cratelyn - I've added the PrimaryMap implementation and moved the validation logic out of main, along with using DictionaryName instead of String as the keys in the Dictionaries HashMap

I think this is ready for another round of reviewing now 😄

aturon · 2021-08-10T21:56:45Z

I think this is ready for another round of reviewing now 😄

Huzzah! I've got the review baton for this next round, and will do that first thing tomorrow morning (Pacific). Thanks again for this work!

aturon

Thanks again for taking this on! I made a review pass and left a few suggestions.

FWIW, I do think your impulse toward memoization was a good one, though I also agree that hot-reloading is nice. I think loading the file on every get is OK for now, but we should file a follow-up issue to preload dictionary contents and use file-watching to asynchronously reload on changes. That'll be helpful for people using really big dictionary files.

I didn't have a chance to carefully review the updated TOML loading and schema here. I know @cratelyn has thought more about that so I'll pass the baton back over on that one.

Cheers!

lib/src/config.rs

lib/src/config/dictionaries.rs

lib/src/wiggle_abi/dictionary_impl.rs

lib/src/error.rs

lib/src/config/unit_tests.rs

lib/src/session.rs

JakeChampion · 2021-08-11T23:16:29Z

Thanks for the review @aturon - I have addressed all the issues now and I learnt about "deref coercion" ☺️

The Dictionary Handles are now created on demand and not reused, to match C@E behaviour 👍

I also spotted some .expect("..") which I left by accident and have converted those to errors under the DictionaryConfigError enum.

I believe this is ready for another round of reviews 🙂

aturon

Thank you, the changes look perfect!

I looked over the TOML deserialization now, and just left one tiny request there. Otherwise, I'm very happy with this PR!

That said, I believe that @cratelyn may have one further though re: the TOML schema (future-facing, thinking about "inline" dictionary definitions), but I'm not sure. I'll pass the baton back over to her to finish this out with you!

lib/src/config/unit_tests.rs

aturon · 2021-08-11T23:34:43Z

lib/src/config/dictionaries.rs

+                        let dictionary_max_len: usize = 1000;
+                        let dictionary_item_key_max_len: usize = 256;
+                        let dictionary_item_value_max_len: usize = 8000;


Could we pull these out into a dedicated limits.rs file? We're going to want to build more awareness of C@E limits in Viceroy like this, and having them in one place will make life easier for keeping them up to date.

Done in b75566d (#61) 👍

Should these also go in the fastly-shared crate?

cratelyn

once again, great work! this is looking excellent 😻

i have provided a few more comments, but almost all of these are very minor quibbles. 🙂 this is looking just about complete!

lib/src/config.rs

lib/src/config/dictionaries.rs

lib/Cargo.toml

lib/src/wiggle_abi/dictionary_impl.rs

lib/src/config/dictionaries.rs

cratelyn

one final comment, about structuring the fastly.toml data in a manner that'll be a bit more future-proof ✨

let me know if this part gives you trouble, i am more than happy to provide some help 👯

lib/src/config/unit_tests.rs

This reads a new section from the `local_server` part of fastly.toml named `dictionaries`. The `local_server.dictionaries` section is an array of tables with two fields: name, and file. The name field is a string which is used as the name of the dictionary and is used via the Dictionary.open method. This field uses the same validation rules asthe Fastly API (Name must start with alphabetical and contain only alphanumeric, underscore, and whitespace). The file field is a string which is a file-path pointing to a JSON file which contains all the dictionary items within an object. An example of a full local_server section of fastly.toml now looks like this: ```toml [local_server] [local_server.backends] [local_server.backends.dog] url = "http://localhost:7878/dog-mocks" [local_server.dictionaries] [local_server.dictionaries.a] name="a" file="./a.json" ```

…ame dictionary be faster and avoid re-reading the same file too many times

…_id`

…s platform Currently the limits are: 1000 items per dictionary 256 characters for dictionary item keys 8000 characters for dictionary item values

…nary-handles

This mimics the 'liveness' of edge dictionaries - a user can edit a dictionary file and see the changes immediately without having to restart viceroy

…eld is meant to point to a file

… a &str

… to read the dictionary file

…gth is too small as this is behaviour that C@E does not do

…nto more idiomatic rust

…instead use `ok_or_else`

…ckend definition

…ctionary file is within currently this only supports json, but more formats are planned to be added later on

cratelyn

thank you so much for implementing this @JakeChampion! this is wonderful, wonderful work ✨ ✔️

cratelyn · 2021-08-17T19:31:19Z

ah! it looks like the ci problems are due to formatting. cargo fmt --all should do the trick!

JakeChampion · 2021-08-17T20:16:50Z

ah! it looks like the ci problems are due to formatting. cargo fmt --all should do the trick!

The formatting issues should be fixed now 👍

aturon · 2021-08-17T20:25:08Z

Looks like Clippy is unhappy. If you run make ci locally, that should simulate the CI checks we do, and once that passes we should be good to merge!

JakeChampion · 2021-08-17T20:58:27Z

Looks like Clippy is unhappy. If you run make ci locally, that should simulate the CI checks we do, and once that passes we should be good to merge!

make ci now passes for me 👍

aturon · 2021-08-17T23:50:26Z

Looks like there are some dictionary-related test failures on Windows: https://github.com/fastly/Viceroy/pull/61/checks?check_run_id=3355092000

aturon · 2021-08-18T16:46:53Z

My best guess of what's happening:

TOML deserialization looks for hex escapes like this: https://github.com/alexcrichton/toml-rs/blob/a6420b64d5525d62d07a3b1b7d8e1f9205b91a0f/src/tokens.rs#L388-L391, they start as \u or \U
Windows uses \ in paths
The temp dir in tests like

Viceroy/lib/src/config/unit_tests.rs

Line 110 in ecda15a

file="{}"

happens to start with u or U triggering this escape code, and what follows isn't valid hex.

aturon · 2021-08-18T17:25:13Z

The TOML spec indicates we should be using ' for paths:

Literal strings are surrounded by single quotes. No escaping is performed so what you see is what you get:
path = 'C:\Users\nodejs\templates'

Sidenote, this will be an issue for eventual inline dictionaries.

JakeChampion · 2021-08-19T09:49:15Z

Thanks for looking into the issue @aturon -- I've pushed a commit to fix this and can confirm that the tests pass on windows now 🎉 -- https://github.com/JakeChampion/Viceroy/runs/3369942476?check_suite_focus=true

aturon · 2021-08-20T18:15:36Z

Passed CI and merged! 🎉

Thanks for both doing this work, and following through these rounds of review and CI issues. This is an important improvement that will benefit a lot of people!

JakeChampion mentioned this pull request Aug 9, 2021

Request to have memory_pages (minimum page size) be configurable #56

Closed

cratelyn requested review from cratelyn and aturon August 9, 2021 15:21

JakeChampion commented Aug 9, 2021

View reviewed changes

lib/src/wiggle_abi/dictionary_impl.rs Outdated Show resolved Hide resolved

lib/src/wiggle_abi/dictionary_impl.rs Show resolved Hide resolved

lib/src/wiggle_abi/dictionary_impl.rs Outdated Show resolved Hide resolved

cratelyn suggested changes Aug 10, 2021

View reviewed changes

aturon requested changes Aug 11, 2021

View reviewed changes

JakeChampion requested review from cratelyn and aturon August 15, 2021 22:58

cratelyn reviewed Aug 16, 2021

View reviewed changes

lib/src/config/unit_tests.rs Outdated Show resolved Hide resolved

JakeChampion requested a review from cratelyn August 16, 2021 23:02

JakeChampion added 15 commits August 17, 2021 00:03

memoize reading a local dictionaries json file to make calls to the s…

005c9d2

…ame dictionary be faster and avoid re-reading the same file too many times

remove no required use serde_json

5662469

remove redundant clone of dictionaries within `with_dictionaries_by…

8c722b6

…_id`

Check dictionary definitions are within the allowed limits of Fastly'…

576c25a

…s platform Currently the limits are: 1000 items per dictionary 256 characters for dictionary item keys 8000 characters for dictionary item values

Move dictionary validaton logic into dictionaries module

c83dd0d

use a PrimaryMap to avoid having to manually track the IDs for dictio…

0ec8eb8

…nary-handles

Read the dictionary files on every Dictionary.get call

107e38e

This mimics the 'liveness' of edge dictionaries - a user can edit a dictionary file and see the changes immediately without having to restart viceroy

use a pathbuf instead of a string for Dictionary.file field as the fi…

d8f552c

…eld is meant to point to a file

format the imports

e9a054e

Use the name of the dictionary table as the name for the dictonary

e70788a

remove dead code

2b047a5

correct the comment for the FastlyConfig.dictionaries function

d273601

correct the comment for the DictionariesConfig struct

e378898

avoid copying name by using deref coercion to convert GuestStr into…

92faacd

… a &str

JakeChampion added 10 commits August 17, 2021 00:03

Add custom errors for misformatted dictionary file and not being able…

edd7237

… to read the dictionary file

Extract the dictionary limits into a separate limits file

9350f1e

Do not write zero into the dictionary item buffer when the buffer len…

276590e

…gth is too small as this is behaviour that C@E does not do

refactor setting default values for local backends and dictionaries i…

0fe9aed

…nto more idiomatic rust

use std::fmt instead of core::fmt

915338b

Remove unused field name from Dictionary struct

2ad330c

refactor how we read the dictionary item key to not use unwrap and …

51a32ea

…instead use `ok_or_else`

Move tempfile into dev-dependencies

c4f7549

Correct comment to mention it is a dictionary definition and not a ba…

ed92569

…ckend definition

Add format field to local dictionary to figure out what format the di…

9782342

…ctionary file is within currently this only supports json, but more formats are planned to be added later on

cratelyn previously approved these changes Aug 17, 2021

View reviewed changes

Fix formatting issues via cargo fmt --all

0d641bd

JakeChampion dismissed cratelyn’s stale review via 0d641bd August 17, 2021 20:12

JakeChampion added 2 commits August 17, 2021 21:42

Change from DictionaryFormat::JSON to DictionaryFormat::Json

e2373aa

update lockfile with new dependencies

ecda15a

use single quotes for paths in toml config

728d924

aturon approved these changes Aug 19, 2021

View reviewed changes

JakeChampion requested a review from cratelyn August 19, 2021 21:13

aturon merged commit 9edda3c into fastly:main Aug 20, 2021

cratelyn mentioned this pull request Aug 30, 2021

📑 dictionary lookups can return none #69

Merged

JakeChampion deleted the dict branch September 13, 2021 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add local dictionary support #61

Add local dictionary support #61

JakeChampion commented Aug 9, 2021 •

edited

Loading

cratelyn left a comment

cratelyn Aug 10, 2021

iximeow Aug 10, 2021

JakeChampion commented Aug 10, 2021

aturon commented Aug 10, 2021

aturon left a comment

JakeChampion commented Aug 11, 2021

aturon left a comment

aturon Aug 11, 2021

JakeChampion Aug 15, 2021

cratelyn left a comment

cratelyn left a comment

cratelyn left a comment

cratelyn commented Aug 17, 2021

JakeChampion commented Aug 17, 2021

aturon commented Aug 17, 2021

JakeChampion commented Aug 17, 2021

aturon commented Aug 17, 2021

aturon commented Aug 18, 2021 •

edited

Loading

aturon commented Aug 18, 2021 •

edited

Loading

JakeChampion commented Aug 19, 2021

aturon commented Aug 20, 2021

Add local dictionary support #61

Add local dictionary support #61

Conversation

JakeChampion commented Aug 9, 2021 • edited Loading

cratelyn left a comment

Choose a reason for hiding this comment

cratelyn Aug 10, 2021

Choose a reason for hiding this comment

iximeow Aug 10, 2021

Choose a reason for hiding this comment

JakeChampion commented Aug 10, 2021

aturon commented Aug 10, 2021

aturon left a comment

Choose a reason for hiding this comment

JakeChampion commented Aug 11, 2021

aturon left a comment

Choose a reason for hiding this comment

aturon Aug 11, 2021

Choose a reason for hiding this comment

JakeChampion Aug 15, 2021

Choose a reason for hiding this comment

cratelyn left a comment

Choose a reason for hiding this comment

cratelyn left a comment

Choose a reason for hiding this comment

cratelyn left a comment

Choose a reason for hiding this comment

cratelyn commented Aug 17, 2021

JakeChampion commented Aug 17, 2021

aturon commented Aug 17, 2021

JakeChampion commented Aug 17, 2021

aturon commented Aug 17, 2021

aturon commented Aug 18, 2021 • edited Loading

aturon commented Aug 18, 2021 • edited Loading

JakeChampion commented Aug 19, 2021

aturon commented Aug 20, 2021

JakeChampion commented Aug 9, 2021 •

edited

Loading

aturon commented Aug 18, 2021 •

edited

Loading

aturon commented Aug 18, 2021 •

edited

Loading