AugustNagro / magnum

A 'new look' for database access in Scala
Apache License 2.0
153 stars 10 forks source link

IllegalArgumentException when parsing null value from DB into Option[scala.math.BigDecimal] #20

Closed williamhaw closed 6 months ago

williamhaw commented 7 months ago

Issue

For Option[scala.math.BigDecimal], in line 301 of DbCodec, it delegates to ScalaBigDecimalCodec.readSingle. At Line 281 of DbCodec, rs.getBigDecimal(pos) returns null, which when passed to the constructor of scala.math.BigDecimal causes it to throw IllegalArgumentException.

To reproduce:

Setup of table and data:

CREATE TABLE IF NOT EXISTS test(
    test_id INT NOT NULL,
    my_big_decimal NUMERIC,
    PRIMARY KEY (test_id)
);

INSERT INTO test(test_id) VALUES (1);

Scala representation:

case class Test(@Id() testId: Int, myBigDecimal: Option[BigDecimal])

Query:

connect(ds)(sql"SELECT * FROM test WHERE test_id = 1".query[Test].run())

Result:

IllegalArgumentException("null value for BigDecimal")

Workaround

Declare field as Option[java.math.BigDecimal] instead and convert manually later.

williamhaw commented 7 months ago

If OptionCodec.readSingle() is changed to perform rs.getObject(pos), then call codec.readSingle() only if the result is not null, would this be efficient enough?

AugustNagro commented 6 months ago

Thanks for the report, I'll take a look soon!

On Fri, Feb 16, 2024, 3:45 AM William Haw @.***> wrote:

Would changing OptionCodec.readSingle() to perform rs.getObject, then calling codec.readSingle() only if the result is not null be efficient enough?

— Reply to this email directly, view it on GitHub https://github.com/AugustNagro/magnum/issues/20#issuecomment-1948242586, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQOKNRHLTQSIWI4JUALVMDYT5BGTAVCNFSM6AAAAABDL4LEEGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBYGI2DENJYGY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

AugustNagro commented 6 months ago

If OptionCodec.readSingle() is changed to perform rs.getObject(pos), then call codec.readSingle() only if the result is not null, would this be efficient enough?

This is a good idea; but since we already propagate null in the case of the other type classes, it should be enough to do the same for ScalaBigDecimalCodec.

Please review the above MR, and then I will release v1.1.1

williamhaw commented 6 months ago

LGTM! Thanks!

AugustNagro commented 6 months ago

Released v1.1.1: https://github.com/AugustNagro/magnum/releases/tag/v1.1.1