Fix serialization error when \n new lines are present

[bpurinton]

Resolves https://github.com/firstdraft/learn/issues/2073

Problem

Using an expectation with \n new line characters in the fuzzy_match helper led to an error:

expect(output).to fuzzy_match("\"l appears 1 times\"\n\"o appears 2 times\"\n\"o appears 2 times\"\n\"p appears 1 times\"")

{"class"=>"String", "serialization_error"=>"invalid dumped string; not wrapped with '\"' nor '\"...\".force_encoding(\"...\")' form"}

Solution

From Claude:

The undump method expects a properly escaped Ruby string literal, but the string being passed has newlines in the middle which makes it invalid for undump.

The error occurs because:

1. The expected string in your test contains literal \n characters and escaped quotes
2. When fuzzy_match processes this, it may strip outer quotes but keep inner content
3. The enriched_json serializer sees a string that starts and ends with " and tries to undump it
4. But the string contains actual newline characters which make it invalid for undump

This is indeed an expected error given the current implementation. The unescape_string_double_quotes method in the enriched_json gem is making an incorrect assumption that any string starting and ending with quotes is a valid
dumped string.

---

Important

Fixes serialization error in unescape_string_double_quotes for strings with newlines and updates tests.

• Behavior:
  • Fixes serialization error in unescape_string_double_quotes in expectation_helper_wrapper.rb when handling strings with newlines.
  • Adds test in edge_cases_spec.rb to verify handling of strings with embedded newlines and quotes.
• Misc:
  • Updates version in version.rb from 0.6.1 to 0.6.2.
  • Adds test case in demo.rb for strings with newlines.

^{This description was created by for a41d6bd. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to e80f284 in 46 seconds. Click for details.

• Reviewed 21 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. lib/rspec/enriched_json/expectation_helper_wrapper.rb:87

• Draft comment:
  The use of a begin/rescue block around str.undump effectively handles cases where the string isn’t a valid dumped format. This prevents serialization errors when newline characters are present. Consider whether you want to log the rescued exception (even conditionally) for easier debugging in the future, and be mindful of the potential performance implications when many invalid strings are processed.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_m8OrI3GzvwUujEFD

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[jelaniwoods]

It makes sense that undump is the issue. Can an example be added to demo.rb and/or spec?

[swaathi]

LGTM. Agree with Jelani. I think it's best to add both a spec and an example for this.

Before merge, please bump the gem version so that it can be deployed.

[swaathi]

Is this a safety measure or do we know of an exact case where this would happen?

If an exact case is known, it should be added to the specs.

[jelaniwoods]

I think undump fails anytime that undumping would result in an invalid string

For example:

"\"Hello, World\"".undump => "Hello, World"
"\"Hello, \"World\"".undump => Exception

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 4ba1e49 in 2 minutes and 2 seconds. Click for details.

• Reviewed 61 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 5 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. demo.rb:181

• Draft comment:
  Using 'match' with a string literal may invoke regex matching. If an exact string comparison is intended, consider using 'eq' or clarifying the intent. Also, the third line differs between output ('o appears 2 times') and expected ('e appears 1 times'); please add a comment if this mismatch is intentional.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a test file specifically demonstrating RSpec matcher failures. The differences between actual and expected values are intentional to show how the matcher behaves when tests fail. The use of 'match' vs 'eq' is also likely intentional as this is in the "String Matchers" section demonstrating the 'match' matcher. The comment misunderstands the purpose of this code. Could this be a legitimate bug in the test where they accidentally used 'match' instead of 'eq'? Maybe the string difference is unintentional? No - looking at the full context, this is clearly in a section deliberately showing different matchers and their failure cases. The file is titled "Demo script for RSpec::EnrichedJson" and contains many intentionally failing tests. The comment should be deleted because it misunderstands that this is a demonstration file intentionally showing failing test cases with different matchers.

2. demo.rb:396

• Draft comment:
  The new test description 'strings with newlines' added to the 'String Matchers' category looks correct.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

3. spec/edge_cases_spec.rb:80

• Draft comment:
  In the 'handling strings with newlines' context, the test uses a begin/rescue block to validate serialization details if an error is raised. For clarity, consider explicitly asserting the expected behavior when no error is raised or using 'expect { ... }.to raise_error' if an error is expected.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The file is specifically testing error handling and serialization details. The begin/rescue pattern is used consistently throughout to verify error details. The suggested change would actually make the tests less clear since we're specifically testing the error case details. The comment seems to misunderstand the purpose of these tests. Perhaps the test could be more explicit about whether it expects the error or not. The comment raises a valid point about test clarity. The consistent pattern throughout the file and the detailed error checking make it clear this is intentionally testing error handling details. Changing the structure would make it harder to test the specific error properties. The comment should be deleted as it suggests changing a intentional and consistent test pattern that is appropriate for this specific edge case testing scenario.

4. spec/edge_cases_spec.rb:40

• Draft comment:
  Defining the constant 'BadObject' within the test may leak into the global namespace. Consider using an anonymous class or isolating it to prevent potential side effects.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. spec/edge_cases_spec.rb:88

• Draft comment:
  Typographical note: The comment uses "undump errors". It might be clearer to say "dumped errors" or another appropriate term to maintain consistency with the error message in line 82.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a very minor suggestion about wording in a test comment. The terms "undump errors" vs "dumped errors" don't materially affect the test's functionality or clarity. The comment is technically about changed code since this is a new test, but it's an extremely low-value suggestion that doesn't improve code quality. The comment does point out an inconsistency in terminology which could theoretically cause confusion for future readers. While technically correct, this level of nitpicking on test comments violates our rule about not making purely informative comments that don't require clear code changes. Test comments are primarily for developer context. Delete this comment as it's too minor and doesn't suggest a necessary code change - it's just about wording in a test comment.

Workflow ID: wflow_QO7xdj8LqRlXbdlZ

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[bpurinton]

@swaathi, I suppose when this is merged there will be some step to bump the gem in the Lambda function as well?

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed a41d6bd in 45 seconds. Click for details.

• Reviewed 12 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. lib/rspec/enriched_json/version.rb:5

• Draft comment:
  Version bump update to 0.6.2; ensure changelog reflects the serialization fix.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_tdtbRjHlRfYdqCEG

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[swaathi]

Yes after the gem is deployed.

I can deploy the gem and update the Lambda. Let me know!

[jelaniwoods]

LGTM

[bpurinton]

Yes after the gem is deployed.

I can deploy the gem and update the Lambda. Let me know!

Merged! You can deploy the gem and make the Lambda update. Thanks.

[raghubetina]

I think we should go ahead with this, but a note: over in #12 I tried delegating serialization completely to Oj, which seems to work very well and dramatically simplifies our code.

The catch is that the "client" then needs to also use Oj for deserialization. But I think that's reasonable.

[bpurinton]

I think we should go ahead with this, but a note: over in #12 I tried delegating serialization completely to Oj, which seems to work very well and dramatically simplifies our code.

The catch is that the "client" then needs to also use Oj for deserialization. But I think that's reasonable.

Nice, made an issue to track #13