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

Encoding of Numeric inside a custom type panics #39

Open
tbo opened this issue Sep 6, 2021 · 4 comments
Open

Encoding of Numeric inside a custom type panics #39

tbo opened this issue Sep 6, 2021 · 4 comments

Comments

@tbo
Copy link

tbo commented Sep 6, 2021

I receive a panic when I try to insert entries with custom types containing Numeric fields. The relevant schema looks like this:

CREATE TYPE product_dimension AS (
  type TEXT,
  unit TEXT,
  value NUMERIC(8,2)
);

CREATE TABLE product (
  [...]
  dimensions product_dimension[],
  [...]
);

I get the following error message If I try to insert a row with value = 55.44:

panic encode []ProductDimension: cannot convert {5044 -2 2 false} to Numeric

I guess he is trying to convert a Numeric struct to a Numeric struct, but I might be wrong. This is the generated code that causes the panic:

func (tr *typeResolver) newProductDimensionArrayInit(ps []ProductDimension) pgtype.ValueTranscoder {
	dec := tr.newProductDimensionArray()
	if err := dec.Set(tr.newProductDimensionArrayRaw(ps)); err != nil {
		panic("encode []ProductDimension: " + err.Error()) // should always succeed
	}
	return textPreferrer{ValueTranscoder: dec, typeName: "_product_dimension"}
}

Considering that the comment says should always succeed I'm inclined to think that this might be a bug.

@jschaf
Copy link
Owner

jschaf commented Sep 6, 2021

Thanks for the detailed report. I can reproduce. The error is that pgtype.Numeric cannot be Set from another pgtype.Numeric instance. This doesn't occur with other numeric types because they're mapped to a literal type. For example, pgtype.Int8 uses the Go int64 scalar type.

panic encode []ProductDimension: cannot convert {5044 -2 2 false} to Numeric

That's the Go representation of a Numeric type, 5044e-2 (assuming you meant 50.44 instead of 55.44).

Here's the minimal reproducible case:

func TestNewQuerier_SetNumericType(t *testing.T) {
	num := &pgtype.Numeric{}
	require.NoError(t, num.Set(64))
	decoder := &pgtype.Numeric{}
	err := decoder.Set(num)
	require.NoError(t, err) // same error here
}

I'm not sure if this is intended behavior by pgtype. I expected that pgtype.Numeric could be Set from another pgtype.Numeric but I'll file a bug upstream to make sure.

As a workaround, I tried to set --gotype 'numeric=*string', so you could use a string type for numeric. That didn't work. I'll take a look at making that work since that's a clear bug in pggen.

@jschaf
Copy link
Owner

jschaf commented Sep 6, 2021

So, we're a bit stuck. Here's the options:

  • We can't use pgtype.Numeric because pgtype.Numeric.Set doesn't support being set from another instance of pgtype.Numeric.

  • --gotype numeric=*string doesn't work because AssignTo doesn't support assigning to a string pointer, which pggen relies on for decoding. Curiously, pgtype.Numeric.AssignTo doesn't support strings, but pgtype.Numeric.Set does support strings. I'll open a PR to add assigning a numeric to *string but that blocks the fix of decoding to a string.

  • --gotype numeric=float64 will work but you might lose precision.

  • --gotype numeric=int64 will work but only for whole numbers. It fails on any numeric with a fractional part.

Short term, you're best bet is probably to rely on an external type and register the encoder and decoder: https://github.com/jackc/pgx/wiki/Numeric-and-decimal-support.

Medium term, --gotype numeric=*string should work for all cases but is blocked by merging a PR into pgtype for pgtype.Numeric.AssignTo. I'd like to avoid requiring an external library.

@jschaf jschaf closed this as completed in 7f87888 Sep 7, 2021
@jschaf jschaf reopened this Sep 7, 2021
@jschaf
Copy link
Owner

jschaf commented Sep 7, 2021

I added an example of how to make this work with a real Decimal type in 7f87888. The main trick is to register the data type on the Querier:

	q := NewQuerierConfig(conn, QuerierConfig{
		DataTypes: []pgtype.DataType{
			{
				Value: &shopspring.Numeric{},
				Name:  "numeric",
				OID:   pgtype.NumericOID,
			},
		},
	})

And then, when invoking pggen, use: --go-type numeric=github.com/shopspring/decimal.Decimal.

The pgx recommendation below won't work because there's not a way to get registered DataTypes of all types of pgx connections, including pgx.Conn, and pgxpool.Pool. The pgx recommendation for pgxpool is to use an AfterConnect hook to call RegisterDataType but pggen can't "see" that occur.

// pgx way that won't work with pggen.
conn.ConnInfo().RegisterDataType(pgtype.DataType{
  Value: &shopspring.Numeric{},
  Name:  "numeric",
  OID:   pgtype.NumericOID,
})

@tbo
Copy link
Author

tbo commented Sep 7, 2021

@jschaf Thank you for your quick and detailed feedback. I was already using float64 as input, so the workaround with --gotype numeric=float64 didn't come with any additional loss of precision.

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

No branches or pull requests

2 participants