Skip to content

Fix: Correctly decode jsonb to String by stripping version byte #568

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

Merged
merged 2 commits into from
Jul 8, 2025

Conversation

nikodittmar
Copy link
Contributor

resolves #562

Description

This pull request resolves an issue where decoding a jsonb column into a String would incorrectly add a \u{01} version byte to the resulting string.

Changes

  • Added a failing test case to reproduce the bug, which now passes.
  • Added a specific case for (.binary, .jsonb) to the PostgresDecodable conformance for String within String+PostgresCodable.swift. The new case reads and discards the version byte before decoding the remainder of the buffer.

Copy link
Collaborator

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Hi @nikodittmar, thanks for this PR!

This looks great! Do you mind adding a unit test for this as well?

The test should live in PostgresNIOTests/New/Data/String+PSQLCodableTests.swift.
Should be something like this:

    func testDecodeFromJSONB() {
        let json = #"{"hello": "world"}"#
        var buffer = ByteBuffer()
        buffer.writeInteger(UInt8(1))
        buffer.writeString(json)

        var decoded: String?
        XCTAssertNoThrow(decoded = try String(from: &buffer, type: .jsonb, format: .binary, context: .default))
        XCTAssertEqual(decoded, json)
    }

@0xTim
Copy link
Member

0xTim commented Jul 7, 2025

This seems to break some tests I have on a client project, including assertion failures like:

XCTAssertEqual failed: ("["bool": false, "array": [true, 4.56, "bar"], "number": 1.23, "object": ["string2": "baz"], "string": "foo", "empty": null]") is not equal to (""{\"bool\": false, \"array\": [true, 4.56, \"bar\"], \"empty\": null, \"number\": 1.23, \"object\": {\"string2\": \"baz\"}, \"string\": \"foo\"}"")

To be clear, this might not be an issue and we were relying of implementations on broken behaviour

Copy link
Collaborator

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

LGTM

@nikodittmar
Copy link
Contributor Author

Hi @fabianfett and @0xTim , thank you both for the feedback!

@fabianfett, I've added the requested unit test in String+PSQLCodableTests.swift.

@0xTim, I believe the failing test might be due to a change in the serialization order of keys in the resulting JSON string. XCTAssertEqual is probably doing a direct string comparison, which is sensitive to key order. JSON doesn't guarantee key order, so potentially a more robust approach would be to both the actual and expected strings into a Codable object or a dictionary and then compare those.

@fabianfett fabianfett merged commit d50aade into vapor:main Jul 8, 2025
9 checks passed
@fabianfett fabianfett added the semver-minor Adds new public API. label Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When loading data type as jsonb, an extra unicode will be added: \u0001
3 participants