Skip to content

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
@fabianfett fabianfett added semver-patch No public API change. and removed semver-minor Adds new public API. labels Aug 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch No public API change.
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