Skip to content

Commit

Permalink
fix: Allow Elixir projects to read flags from a local file (#106)
Browse files Browse the repository at this point in the history
**Requirements**

- [x] I have added test coverage for new or changed functionality
- [x] I have followed the repository's [pull request submission
guidelines](../blob/main/CONTRIBUTING.md#submitting-pull-requests)
- [ ] I have validated my changes against all supported platform
versions

**Context**

While trying to configure our Elixir service to read feature flags from
a local file (for local development purposes), we ran into an issue
where the server SDK would throw an "unsupported extension" error, even
though the configured path pointed to a YAML file.

**Root cause**

Erlang uses charlists to represent strings, while Elixir uses binaries.
So when we pass a path to a YAML file to the `ldclient` config in our
Elixir project, the Erlang function that parses the file extension on
the SDK side receives a binary, which it does not support. Thus, the SDK
throws an error claiming that we're using an invalid file type.

**Describe the solution you've provided**

This PR adds a helper function that handles binary type strings. If the
`FilePath` is a binary, we simply convert it to a charlist.

---------

Co-authored-by: Luis Beligante <luis.beligante@thescore.com>
  • Loading branch information
kaifee-haque and beligante committed Nov 17, 2023
1 parent b709a19 commit c290599
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 12 deletions.
2 changes: 1 addition & 1 deletion src/ldclient_config.erl
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@
use_ldd => boolean(),
send_events => boolean(),
file_datasource => boolean(),
file_paths => [string()],
file_paths => [string() | binary()],
file_auto_update => boolean(),
file_poll_interval => pos_integer(),
file_allow_duplicate_keys => boolean(),
Expand Down
2 changes: 1 addition & 1 deletion src/ldclient_instance.erl
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
cache_ttl => integer(),
send_events => boolean(),
file_datasource => boolean(),
file_paths => [string()],
file_paths => [string() | binary()],
file_auto_update => boolean(),
file_poll_interval => pos_integer(),
file_allow_duplicate_keys => boolean(),
Expand Down
26 changes: 16 additions & 10 deletions src/ldclient_update_file_server.erl
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
-export([code_change/3, handle_call/3, handle_cast/2, handle_info/2, terminate/2]).

-type state() :: #{
file_paths := [string()],
file_paths := [string() | binary()],
file_auto_update := boolean(),
file_poll_interval := pos_integer(),
file_allow_duplicate_keys := boolean(),
Expand Down Expand Up @@ -155,7 +155,7 @@ upsert_valid_flag_data(true, Tag, ParsedFlags, ParsedSegments) ->
upsert_valid_flag_data(false, _Tag, _ParsedFlags, _ParsedSegments) ->
error.

-spec read_file(FilePath :: string(), Extension :: string()) -> {ok | error, map()}.
-spec read_file(FilePath :: string() | binary(), Extension :: string()) -> {ok | error, map()}.
read_file(FilePath, ".yaml") ->
Result = yamerl_constr:file(FilePath, [{detailed_constr, true}]),
[Head | _] = ldclient_yaml_mapper:to_map_docs(Result, []),
Expand All @@ -167,7 +167,7 @@ read_file(FilePath, _Extension) ->
error_logger:warning_msg("File had unrecognized file extension. Valid extensions are .yaml and .json. File: ~p", [FilePath]),
{error, #{}}.

-spec try_read_file(FilePath :: string(), Extension :: string()) -> {ok | error, map()}.
-spec try_read_file(FilePath :: string() | binary(), Extension :: string()) -> {ok | error, map()}.
try_read_file(FilePath, Extension) ->
try
read_file(FilePath, Extension)
Expand All @@ -176,7 +176,7 @@ try_read_file(FilePath, Extension) ->
{error, #{}}
end.

-spec load_file(FilePath :: string(), Extension :: string(), State :: state(), LoadedFlags :: map(), LoadedSegments :: map(), FlagCount :: non_neg_integer()) ->
-spec load_file(FilePath :: string() | binary(), Extension :: string(), State :: state(), LoadedFlags :: map(), LoadedSegments :: map(), FlagCount :: non_neg_integer()) ->
{ok | error, NewState :: state(), LoadedFlags :: map(), LoadedSegments :: map(), FlagCount :: non_neg_integer()}.
load_file(FilePath, Extension, #{file_info := CurFileInfo} = State, LoadedFlags, LoadedSegments, FlagCount) ->
ModifiedTime = filelib:last_modified(FilePath),
Expand All @@ -196,7 +196,7 @@ process_decoded_file(ok, Decoded, State, LoadedFlags, LoadedSegments, FlagCount)
process_decoded_file(error, _Decoded, State, LoadedFlags, LoadedSegments, FlagCount) ->
{error, State, LoadedFlags, LoadedSegments, FlagCount}.

-spec check_modified(FilesToCheck :: [string()], Modified :: boolean(), State :: state()) ->
-spec check_modified(FilesToCheck :: [string() | binary()], Modified :: boolean(), State :: state()) ->
{Modified :: boolean(), State :: state()}.
check_modified([], Modified, State) ->
{Modified, State};
Expand All @@ -207,29 +207,29 @@ check_modified([FileToCheck | RestOfFiles], Modified, State) ->
NewModified = Modified or (ExistingModifiedTime =/= ModifiedTime),
check_modified(RestOfFiles, NewModified, State).

-spec load_files_if_modified(Files :: [string()], State :: state()) -> {ok | error, State :: state()}.
-spec load_files_if_modified(Files :: [string() | binary()], State :: state()) -> {ok | error, State :: state()}.
load_files_if_modified(Files, State) ->
case check_modified(Files, false, State) of
{true, UpdatedState} -> load_files(Files, UpdatedState);
{false, UpdatedState} -> {ok, UpdatedState}
end.

-spec load_regular_file(FilePath :: string(), IsRegularFile :: boolean(), State :: state(), LoadedFlags :: map(), LoadedSegments :: map(), FlagCount :: non_neg_integer()) ->
-spec load_regular_file(FilePath :: string() | binary(), IsRegularFile :: boolean(), State :: state(), LoadedFlags :: map(), LoadedSegments :: map(), FlagCount :: non_neg_integer()) ->
{ok | error, NewState :: state(), LoadedFlags :: map(), LoadedSegments :: map(), FlagCount :: non_neg_integer()}.
load_regular_file(FileToLoad, true, State, LoadedFlags, LoadedSegments, FlagCount) ->
load_file(FileToLoad, filename:extension(FileToLoad), State, LoadedFlags, LoadedSegments, FlagCount);
load_regular_file(_FileToLoad, false, State, LoadedFlags, LoadedSegments, FlagCount) ->
{error, State, LoadedFlags, LoadedSegments, FlagCount}.

-spec load_files(Files :: [string()], State :: state()) -> {ok | error, State :: state()}.
-spec load_files(Files :: [string() | binary()], State :: state()) -> {ok | error, State :: state()}.
load_files(Files, State) ->
load_files(Files, State, #{}, #{}, 0, ok).

-spec load_files(Files :: [string()], State :: state(), CombinedFlags :: map(), CombinedSegments :: map(), FlagCount :: non_neg_integer(), Status :: ok | error) ->
-spec load_files(Files :: [string() | binary()], State :: state(), CombinedFlags :: map(), CombinedSegments :: map(), FlagCount :: non_neg_integer(), Status :: ok | error) ->
{ok | error, State :: state()}.
load_files([FileToLoad | RestOfFiles], State, LoadedFlags, LoadedSegments, FlagCount, ok) ->
{NewStatus, NewState, CombinedFlags, CombinedSegments, UpdatedCount} =
load_regular_file(FileToLoad, filelib:is_regular(FileToLoad), State, LoadedFlags, LoadedSegments, FlagCount),
load_regular_file(handle_file_path_type(FileToLoad), filelib:is_regular(handle_file_path_type(FileToLoad)), State, LoadedFlags, LoadedSegments, FlagCount),
load_files(RestOfFiles, NewState, CombinedFlags, CombinedSegments, UpdatedCount, NewStatus);
load_files([], #{file_allow_duplicate_keys := AllowDuplicateKeys, storage_tag := Tag} = State, LoadedFlags, LoadedSegments, FlagCount, ok) ->
Valid = (FlagCount == length(maps:keys(LoadedFlags))) or AllowDuplicateKeys,
Expand All @@ -238,3 +238,9 @@ load_files([], #{file_allow_duplicate_keys := AllowDuplicateKeys, storage_tag :=
load_files(_Files, State, _LoadedFlags, _LoadedSegments, _FlagCount, error) ->
%% If there is an error, then do not process any more files.
{error, State}.

-spec handle_file_path_type(FilePath :: string() | binary()) -> string().
handle_file_path_type(FilePath) when is_binary(FilePath) ->
binary_to_list(FilePath);
handle_file_path_type(FilePath) ->
FilePath.
15 changes: 15 additions & 0 deletions test/ldclient_file_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
check_file_simple_flags_yaml/1,
check_file_all_properties_yaml/1,
check_multiple_data_files/1,
check_binary_file_path/1,
check_file_watcher/1,
check_with_missing_file/1,
check_with_only_missing_file/1,
Expand All @@ -35,6 +36,7 @@ all() ->
check_file_simple_flags_yaml,
check_file_all_properties_yaml,
check_multiple_data_files,
check_binary_file_path,
check_file_watcher,
check_with_missing_file,
check_with_only_missing_file,
Expand Down Expand Up @@ -91,6 +93,14 @@ init_per_suite(Config) ->
},
ldclient:start_instance("", simple_flags_yaml, OptionsSimpleFlagsYaml),

OptionsBinaryPathYaml = #{
datasource => file,
send_events => false,
file_paths => [list_to_binary(DataFileSimpleFlagsYaml)],
feature_store => ldclient_storage_map
},
ldclient:start_instance("", binary_path_yaml, OptionsBinaryPathYaml),

DataFileAllPropertiesYaml = code:priv_dir(ldclient) ++ "/flags-all-properties.yaml",
OptionsAllPropertiesYaml = #{
datasource => file,
Expand Down Expand Up @@ -208,6 +218,11 @@ check_multiple_data_files(_) ->
{{0, true, fallthrough}, _} = ldclient_eval:flag_key_for_context(all_properties_yaml, <<"my-boolean-flag-key">>, #{key => <<"user123">>, kind => <<"user">>}, "foo"),
{{0, 3, fallthrough}, _} = ldclient_eval:flag_key_for_context(all_properties_yaml, <<"my-integer-flag-key">>, #{key => <<"user123">>, kind => <<"user">>}, "foo").

check_binary_file_path(_) ->
{{0, <<"value-1">>, fallthrough}, _} = ldclient_eval:flag_key_for_context(binary_path_yaml, <<"my-string-flag-key">>, #{key => <<"user123">>, kind => <<"user">>}, "foo"),
{{0, true, fallthrough}, _} = ldclient_eval:flag_key_for_context(binary_path_yaml, <<"my-boolean-flag-key">>, #{key => <<"user123">>, kind => <<"user">>}, "foo"),
{{0, 3, fallthrough}, _} = ldclient_eval:flag_key_for_context(binary_path_yaml, <<"my-integer-flag-key">>, #{key => <<"user123">>, kind => <<"user">>}, "foo").

check_file_watcher(_) ->
{{0, true, fallthrough}, _} = ldclient_eval:flag_key_for_context(watch_files, <<"test-flag">>, #{key => <<"user123">>, kind => <<"user">>}, "foo"),
%The timestamp for the modified time is in seconds. So we wait a moment to get a new timestamp compared to creation.
Expand Down

0 comments on commit c290599

Please sign in to comment.