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

add decode option to redact line sequence data from CSV #124

Merged

Conversation

taylor-redden-papa
Copy link
Contributor

This PR addresses
CSVs might contain sensitive data, so we could allow a decode option to not include that data as part of the exception messaging.

Open questions
It could be argued that this isn't needed at all and apps using this library could just not log this out when they get this exception. But for long standing apps, it might be easier for them to add this decode option in the few areas where they need to without changing any other telemetry or logging.

The naming from the option was inspired by the ecto field option redact. I thought this might be a similar situation.

Checklist

  • Tests added ✅
  • Coverage increases or stays the same
  • Docs updated ✅
  • Changelog updated

@taylor-redden-papa
Copy link
Contributor Author

@beatrichartz you think you'll get a chance to look at this any time soon?

@beatrichartz
Copy link
Owner

@taylor-redden-papa thanks, functionality is looking good. There is a bunch of Elixir versions removed from CI, was there a reason for this or can we add them back?

@taylor-redden-papa
Copy link
Contributor Author

Hey, thanks for taking a look at it, I appreciate it.

I can definitely add it back, but I was seeing this error when cicd was running:

https://github.com/beatrichartz/csv/actions/runs/6115205079/job/16598348600

==> dialyxir
warning: the dependency :dialyxir requires Elixir ">= 1.12.0" but you are running on v1.9.4
Compiling 66 files (.ex)

== Compilation error in file lib/dialyxir/project.ex ==
Error: ** (CompileError) lib/dialyxir/project.ex:365: undefined function then/2
    (elixir) expanding macro: Kernel.|>/2
    lib/dialyxir/project.ex:365: Dialyxir.Project (module)
Warning:     (elixir) expanding macro: Kernel.if/2
    lib/dialyxir/project.ex:365: Dialyxir.Project (module)
could not compile dependency :dialyxir, "mix compile" failed. You can recompile this dependency with "mix deps.compile dialyxir", update it with "mix deps.update dialyxir" or clean it with "mix deps.clean dialyxir"
Error: Process completed with exit code 1.

Not sure exactly why that happened, I hadn't see it on any pull requests recently.

Give me a few and I'll change ci.yml back to original.

@beatrichartz
Copy link
Owner

Ah I see dyalixir introduced a non-backwards compatible change. Can you lock it to version 1.3 for this pr?

@coveralls
Copy link

Coverage Status

coverage: 100.0%. remained the same when pulling df74e3e on taylor-redden-papa:redact-exception-better into 13488b1 on beatrichartz:main.

@taylor-redden-papa
Copy link
Contributor Author

That works.

Not sure whats up with coveralls. Made a comment coverage is still good, but the check didn't pass.

@beatrichartz beatrichartz merged commit a3571ff into beatrichartz:main Sep 19, 2023
15 checks passed
@beatrichartz
Copy link
Owner

Thanks, I'll release a new version some time this week!

@taylor-redden-papa
Copy link
Contributor Author

awesome, thanks so much!

@taylor-redden-papa taylor-redden-papa deleted the redact-exception-better branch September 19, 2023 05:24
@beatrichartz
Copy link
Owner

Ok this has been published in 3.2.0 with a few changes:

  • Redacting source data in thrown exceptions is the default behaviour for strict mode. This ensures that showing the data in exception messages is not accidental. It can be turned off using CSV.decode!(stream, unredact_exceptions: true)
  • Normal mode does have an option to redact_errors to enable redacting error messages. Since logging that data would always be an intentional part of a stream pipeline, that option is opt in: CSV.decode(stream, redact_errors: true)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants