Alternative implementation for JSONCodec.PlanScan

[felix-roehrich]

The fix from #2151 just pushed the original problem from #1470 and #1691 further along. This should be a comprehensive solution; also resolves #2229.

[felix-roehrich]

I will add a test to prevent regression from #2229 and look at failing tests tomorrow.

[felix-roehrich]

@jackc Looking at the following examples, I believe that the current scanning behavior is in inconsistent.

type Foo struct{}

var dst *Foo
err = conn.QueryRow(context.Background(), `select 'null'::jsonb`).Scan(&dst)
// err == nil
// dst == nil

var dst *Foo
err = conn.QueryRow(context.Background(), `select null::jsonb`).Scan(&dst)
// err == nil
// dst == nil

var dst Foo
err = conn.QueryRow(context.Background(), `select 'null'::jsonb`).Scan(&dst)
// err == nil
// dst == Foo{}

var dst Foo
err = conn.QueryRow(context.Background(), `select null::jsonb`).Scan(&dst)
// err == can't scan into dest[0] (col: jsonb): cannot scan NULL into *pgx_test.Foo
// dst == Foo{}

I believe either both cases of scanning Foo should error or none (or change scanning of *Foo). What is your opinion?

[felix-roehrich]

I decided to revert #2151 partially. This is because this should never be necessary, as such cases would be handled by the PointerPointerScanPlan. JSON requires special care to handle the scanning of 'null'::jsonb.

[jackc]

Thanks! sql.Scanner especially plus json has been such a pain. Hopefully, this finally resolves it.

[emreisikligil]

@jackc Can we add this to the next patch version v5.7.3 please as v5.7.2 has the regression when scanning null jsonb columns into a pointer?

[jackc]

@emreisikligil Just released v5.7.3.

[emreisikligil]

@jackc Thanks a lot 🙏

[alicebob]

Just as a FYI, this did change the behavior of scanning a constant null into a json.RawMessage. If I pass in a jSON value null, then I don't get back what I put into it, but a NULL/nil. I don't know the whole history of this change, and I'm sure it's needed somehow, so I put this comment here as a warning/gotcha, maybe it helps someone:

    ["CREATE TABLE pgx (jsoncol JSONB NOT NULL DEFAULT 'null')"]
    ["INSERT INTO pgx (jsoncol) VALUES ('null')"]
    var j json.RawMessage
    require.NoError(t, db.QueryRow(ctx, `SELECT jsoncol FROM pgx`).Scan(&j))
    require.Equal(t, json.RawMessage([]byte("null")), j)

this now fails in 5.7.3 with:

        	Error:      	Not equal: 
        	            	expected: json.RawMessage{0x6e, 0x75, 0x6c, 0x6c}
        	            	actual  : json.RawMessage(nil)

The test passes on 5.7.2.

[felix-roehrich]

This is due to this change. Which was made with #2236 (comment) in mind, however looking at the godoc for the json package, not erroring is intentional. Though I personally don't agree with the rational, it's probably for the best to revert this change.

// The JSON null value unmarshals into an interface, map, pointer, or slice
// by setting that Go value to nil. Because null is often used in JSON to mean
// “not present,” unmarshaling a JSON null into any other Go type has no effect
// on the value and produces no error.

[jackc]

I agree. I think this should be reverted.

[alicebob]

Thanks both for the quick reply, and many more thanks for a great library.