Skip to content

fix: improve yaml load for sinks.max_retry_count, sinks.active_backfill and http_endpoints.encrypted_headers#1432

Merged
acco merged 3 commits intosequinstream:mainfrom
davoclavo:fix/improve_yaml_loading
May 12, 2025
Merged

fix: improve yaml load for sinks.max_retry_count, sinks.active_backfill and http_endpoints.encrypted_headers#1432
acco merged 3 commits intosequinstream:mainfrom
davoclavo:fix/improve_yaml_loading

Conversation

@davoclavo
Copy link
Contributor

@davoclavo davoclavo commented May 12, 2025

While testing out CLI export->plan functionality, I detected a couple of issues when trying to load a config from a yaml.

  1. sinks.max_retry_count - if is set to nil (as it seems its the default) the loader fails with an error stating that it should be greater than 0
  2. sinks.active_backfill - if is present, and set to any value, the loader fails. I suggest to ignore this field for now until we support managing backfills via the yaml.
  3. http_endpoints.encrypted_headers - seems to be serialized as a string/binary (even if the map is empty, as it seems is the default) - causing to fail parse_headers validation. I suggest ignoring this field for now, unless we want to handle encrypting/decrypting them properly
    3.1. I also made a change to match if encrypted headers is an empty map, as opposed to nil and serialize them as an empty map

I added a simple test that includes a single resource of each, mostly created with default settings.

For a future iteration of an improved but similar check I would suggest to write a test that creates a bunch of records and combinations (databases, change_retentions, sinks, http_endpoint, functions); then assert that exporting->loading works. That way we can test that if the exporting/serializing logic changes, it is still compatible with the loading/deserializing logic and viceversa.

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label May 12, 2025
@davoclavo davoclavo force-pushed the fix/improve_yaml_loading branch from 28c6b5d to 1228857 Compare May 12, 2025 21:31
@dosubot dosubot bot added the bug Something isn't working label May 12, 2025
As handling `active_backfill` seems to be entirely unsupported we are ignoring
that field for now until we have a proper implementation
@davoclavo davoclavo force-pushed the fix/improve_yaml_loading branch from 1228857 to 1b90bec Compare May 12, 2025 21:33
@davoclavo davoclavo requested a review from Copilot May 12, 2025 21:35
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves the YAML configuration loader so that problematic fields do not cause failures during configuration load.

  • Ignores nil values for max_retry_count instead of erroring.
  • Temporarily ignores active_backfill and encrypted_headers until proper support is implemented.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
test/sequin_web/controllers/yaml_controller_test.exs Adds a comprehensive test case covering various configuration fields.
lib/sequin/transforms/transforms.ex Updates handling for encrypted_headers, max_retry_count (nil), and active_backfill based on current requirements.

@davoclavo davoclavo force-pushed the fix/improve_yaml_loading branch from 735b756 to 8717dd0 Compare May 12, 2025 22:08
@davoclavo davoclavo force-pushed the fix/improve_yaml_loading branch from 8717dd0 to 6f6cc76 Compare May 12, 2025 22:14
@davoclavo
Copy link
Contributor Author

davoclavo commented May 12, 2025

I just noticed that some tests are failing that are testing proper loading for encrypted_headers - however I don't currently see a case where they could be serialized as a map (as it states in the test - see link below)

  • What is the current state of handling encrypted_headers??
  • If we don't support them for now, is it OK if I adjust these tests to reflect that they will be ignored from load for now?

Example test https://github.com/sequinstream/sequin/blob/main/test/sequin/yaml_loader_test.exs#L334-L343

Comment on lines +406 to +409
defp encrypted_headers(%HttpEndpoint{encrypted_headers: encrypted_headers})
when is_map(encrypted_headers) and map_size(encrypted_headers) == 0,
do: %{}

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice addition

Comment on lines +438 to +440
# Ignore encrypted_headers until encryption/decryption works
"encrypted_headers" ->
case parse_headers(value) do
{:ok, headers} -> {:cont, {:ok, Map.put(acc, :encrypted_headers, headers)}}
{:error, error} -> {:halt, {:error, error}}
end
{:cont, {:ok, acc}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to revert this before merging.

In our sequin config export, we mention that some fields are not supported - this is one of them. You're right, we export is as a binary, just a sha of the secret. This is not ideal, but users are definitely using this field for their imports.

We want to improve secret handling here: #1298

Comment on lines +602 to +603
"max_retry_count" when is_nil(value) ->
{:cont, {:ok, acc}}
Copy link
Contributor

Choose a reason for hiding this comment

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

great catch!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label May 12, 2025
@acco acco force-pushed the fix/improve_yaml_loading branch from 81d3348 to 73b16b6 Compare May 12, 2025 23:51
@acco acco force-pushed the fix/improve_yaml_loading branch from 73b16b6 to 935deb8 Compare May 12, 2025 23:53
@acco
Copy link
Contributor

acco commented May 12, 2025

@davoclavo note I also fixed the tests -- a lot of our database create paths actually connect to the db and check its configuration. So we need to reference test tables that we know are in the database (see test/support/unboxed_repo/migrations/20240816005458_create_test_tables.exs and an example model in test/support/models/character.ex)

So, I modified to refer to existing tables:
935deb8#diff-6c8c71c18120f4015b13a5e5560426e89afc5e883b7af3f33c1beb13fcaf7bca

@acco acco merged commit ebfbbf1 into sequinstream:main May 12, 2025
1 of 2 checks passed
@davoclavo
Copy link
Contributor Author

@acco Thanks for letting me know and making some changes!

I just want to point out that there are still issues with the encrypted headers, so I am unsure people are able to use them for their imports currently!

Just added steps to replicate in #1442 in order to get to the bottom of this. I will start my investigation, but let me know if you have any pointers, or if you make any progress on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants