Skip to content

Commit 2522c02

Browse files
BaccataBaccata
authored
Merge pull request #1624 from disneystreaming/nan-edge-cases
Handle edge cases with NaN/Infinity
2 parents e369dd4 + 3b91604 commit 2522c02

File tree

10 files changed

+173
-34
lines changed

10 files changed

+173
-34
lines changed

‎CHANGELOG.md

+4
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,10 @@ When adding entries, please treat them as if they could end up in a release any
55

66
Thank you!
77

8+
# 0.18.27
9+
10+
* Fix for how `NaN` is handled for `Float` and `Double` inside of the `MetadataDecoder` and `Range` constraint `RefinementProvider`
11+
812
# 0.18.26
913

1014
* Optimises the conversion of empty smithy4s.Blob to fs2.Stream, to avoid performance degradation in Ember (see [#1609](https://github.com/disneystreaming/smithy4s/pull/1609))

‎modules/bootstrapped/src/generated/smithy4s/example/DummyService.scala

+4-4
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import smithy4s.schema.Schema.unit
1818
trait DummyServiceGen[F[_, _, _, _, _]] {
1919
self =>
2020

21-
def dummy(str: Option[String] = None, int: Option[Int] = None, ts1: Option[Timestamp] = None, ts2: Option[Timestamp] = None, ts3: Option[Timestamp] = None, ts4: Option[Timestamp] = None, b: Option[Boolean] = None, sl: Option[List[String]] = None, ie: Option[Numbers] = None, on: Option[OpenNums] = None, ons: Option[OpenNumsStr] = None, slm: Option[Map[String, String]] = None): F[Queries, Nothing, Unit, Nothing, Nothing]
21+
def dummy(str: Option[String] = None, int: Option[Int] = None, ts1: Option[Timestamp] = None, ts2: Option[Timestamp] = None, ts3: Option[Timestamp] = None, ts4: Option[Timestamp] = None, b: Option[Boolean] = None, sl: Option[List[String]] = None, ie: Option[Numbers] = None, on: Option[OpenNums] = None, ons: Option[OpenNumsStr] = None, dbl: Option[Double] = None, slm: Option[Map[String, String]] = None): F[Queries, Nothing, Unit, Nothing, Nothing]
2222
def dummyHostPrefix(label1: String, label2: String, label3: HostLabelEnum): F[HostLabelInput, Nothing, Unit, Nothing, Nothing]
2323
def dummyPath(str: String, int: Int, ts1: Timestamp, ts2: Timestamp, ts3: Timestamp, ts4: Timestamp, b: Boolean, ie: Numbers): F[PathParams, Nothing, Unit, Nothing, Nothing]
2424

@@ -69,12 +69,12 @@ sealed trait DummyServiceOperation[Input, Err, Output, StreamedInput, StreamedOu
6969
object DummyServiceOperation {
7070

7171
object reified extends DummyServiceGen[DummyServiceOperation] {
72-
def dummy(str: Option[String] = None, int: Option[Int] = None, ts1: Option[Timestamp] = None, ts2: Option[Timestamp] = None, ts3: Option[Timestamp] = None, ts4: Option[Timestamp] = None, b: Option[Boolean] = None, sl: Option[List[String]] = None, ie: Option[Numbers] = None, on: Option[OpenNums] = None, ons: Option[OpenNumsStr] = None, slm: Option[Map[String, String]] = None): Dummy = Dummy(Queries(str, int, ts1, ts2, ts3, ts4, b, sl, ie, on, ons, slm))
72+
def dummy(str: Option[String] = None, int: Option[Int] = None, ts1: Option[Timestamp] = None, ts2: Option[Timestamp] = None, ts3: Option[Timestamp] = None, ts4: Option[Timestamp] = None, b: Option[Boolean] = None, sl: Option[List[String]] = None, ie: Option[Numbers] = None, on: Option[OpenNums] = None, ons: Option[OpenNumsStr] = None, dbl: Option[Double] = None, slm: Option[Map[String, String]] = None): Dummy = Dummy(Queries(str, int, ts1, ts2, ts3, ts4, b, sl, ie, on, ons, dbl, slm))
7373
def dummyHostPrefix(label1: String, label2: String, label3: HostLabelEnum): DummyHostPrefix = DummyHostPrefix(HostLabelInput(label1, label2, label3))
7474
def dummyPath(str: String, int: Int, ts1: Timestamp, ts2: Timestamp, ts3: Timestamp, ts4: Timestamp, b: Boolean, ie: Numbers): DummyPath = DummyPath(PathParams(str, int, ts1, ts2, ts3, ts4, b, ie))
7575
}
7676
class Transformed[P[_, _, _, _, _], P1[_ ,_ ,_ ,_ ,_]](alg: DummyServiceGen[P], f: PolyFunction5[P, P1]) extends DummyServiceGen[P1] {
77-
def dummy(str: Option[String] = None, int: Option[Int] = None, ts1: Option[Timestamp] = None, ts2: Option[Timestamp] = None, ts3: Option[Timestamp] = None, ts4: Option[Timestamp] = None, b: Option[Boolean] = None, sl: Option[List[String]] = None, ie: Option[Numbers] = None, on: Option[OpenNums] = None, ons: Option[OpenNumsStr] = None, slm: Option[Map[String, String]] = None): P1[Queries, Nothing, Unit, Nothing, Nothing] = f[Queries, Nothing, Unit, Nothing, Nothing](alg.dummy(str, int, ts1, ts2, ts3, ts4, b, sl, ie, on, ons, slm))
77+
def dummy(str: Option[String] = None, int: Option[Int] = None, ts1: Option[Timestamp] = None, ts2: Option[Timestamp] = None, ts3: Option[Timestamp] = None, ts4: Option[Timestamp] = None, b: Option[Boolean] = None, sl: Option[List[String]] = None, ie: Option[Numbers] = None, on: Option[OpenNums] = None, ons: Option[OpenNumsStr] = None, dbl: Option[Double] = None, slm: Option[Map[String, String]] = None): P1[Queries, Nothing, Unit, Nothing, Nothing] = f[Queries, Nothing, Unit, Nothing, Nothing](alg.dummy(str, int, ts1, ts2, ts3, ts4, b, sl, ie, on, ons, dbl, slm))
7878
def dummyHostPrefix(label1: String, label2: String, label3: HostLabelEnum): P1[HostLabelInput, Nothing, Unit, Nothing, Nothing] = f[HostLabelInput, Nothing, Unit, Nothing, Nothing](alg.dummyHostPrefix(label1, label2, label3))
7979
def dummyPath(str: String, int: Int, ts1: Timestamp, ts2: Timestamp, ts3: Timestamp, ts4: Timestamp, b: Boolean, ie: Numbers): P1[PathParams, Nothing, Unit, Nothing, Nothing] = f[PathParams, Nothing, Unit, Nothing, Nothing](alg.dummyPath(str, int, ts1, ts2, ts3, ts4, b, ie))
8080
}
@@ -83,7 +83,7 @@ object DummyServiceOperation {
8383
def apply[I, E, O, SI, SO](op: DummyServiceOperation[I, E, O, SI, SO]): P[I, E, O, SI, SO] = op.run(impl)
8484
}
8585
final case class Dummy(input: Queries) extends DummyServiceOperation[Queries, Nothing, Unit, Nothing, Nothing] {
86-
def run[F[_, _, _, _, _]](impl: DummyServiceGen[F]): F[Queries, Nothing, Unit, Nothing, Nothing] = impl.dummy(input.str, input.int, input.ts1, input.ts2, input.ts3, input.ts4, input.b, input.sl, input.ie, input.on, input.ons, input.slm)
86+
def run[F[_, _, _, _, _]](impl: DummyServiceGen[F]): F[Queries, Nothing, Unit, Nothing, Nothing] = impl.dummy(input.str, input.int, input.ts1, input.ts2, input.ts3, input.ts4, input.b, input.sl, input.ie, input.on, input.ons, input.dbl, input.slm)
8787
def ordinal: Int = 0
8888
def endpoint: smithy4s.Endpoint[DummyServiceOperation,Queries, Nothing, Unit, Nothing, Nothing] = Dummy
8989
}

‎modules/bootstrapped/src/generated/smithy4s/example/Queries.scala

+4-2
Original file line numberDiff line numberDiff line change
@@ -6,20 +6,21 @@ import smithy4s.ShapeId
66
import smithy4s.ShapeTag
77
import smithy4s.Timestamp
88
import smithy4s.schema.Schema.boolean
9+
import smithy4s.schema.Schema.double
910
import smithy4s.schema.Schema.int
1011
import smithy4s.schema.Schema.string
1112
import smithy4s.schema.Schema.struct
1213
import smithy4s.schema.Schema.timestamp
1314

14-
final case class Queries(str: Option[String] = None, int: Option[Int] = None, ts1: Option[Timestamp] = None, ts2: Option[Timestamp] = None, ts3: Option[Timestamp] = None, ts4: Option[Timestamp] = None, b: Option[Boolean] = None, sl: Option[List[String]] = None, ie: Option[Numbers] = None, on: Option[OpenNums] = None, ons: Option[OpenNumsStr] = None, slm: Option[Map[String, String]] = None)
15+
final case class Queries(str: Option[String] = None, int: Option[Int] = None, ts1: Option[Timestamp] = None, ts2: Option[Timestamp] = None, ts3: Option[Timestamp] = None, ts4: Option[Timestamp] = None, b: Option[Boolean] = None, sl: Option[List[String]] = None, ie: Option[Numbers] = None, on: Option[OpenNums] = None, ons: Option[OpenNumsStr] = None, dbl: Option[Double] = None, slm: Option[Map[String, String]] = None)
1516

1617
object Queries extends ShapeTag.Companion[Queries] {
1718
val id: ShapeId = ShapeId("smithy4s.example", "Queries")
1819

1920
val hints: Hints = Hints.empty
2021

2122
// constructor using the original order from the spec
22-
private def make(str: Option[String], int: Option[Int], ts1: Option[Timestamp], ts2: Option[Timestamp], ts3: Option[Timestamp], ts4: Option[Timestamp], b: Option[Boolean], sl: Option[List[String]], ie: Option[Numbers], on: Option[OpenNums], ons: Option[OpenNumsStr], slm: Option[Map[String, String]]): Queries = Queries(str, int, ts1, ts2, ts3, ts4, b, sl, ie, on, ons, slm)
23+
private def make(str: Option[String], int: Option[Int], ts1: Option[Timestamp], ts2: Option[Timestamp], ts3: Option[Timestamp], ts4: Option[Timestamp], b: Option[Boolean], sl: Option[List[String]], ie: Option[Numbers], on: Option[OpenNums], ons: Option[OpenNumsStr], dbl: Option[Double], slm: Option[Map[String, String]]): Queries = Queries(str, int, ts1, ts2, ts3, ts4, b, sl, ie, on, ons, dbl, slm)
2324

2425
implicit val schema: Schema[Queries] = struct(
2526
string.optional[Queries]("str", _.str).addHints(smithy.api.HttpQuery("str")),
@@ -33,6 +34,7 @@ object Queries extends ShapeTag.Companion[Queries] {
3334
Numbers.schema.optional[Queries]("ie", _.ie).addHints(smithy.api.HttpQuery("nums")),
3435
OpenNums.schema.optional[Queries]("on", _.on).addHints(smithy.api.HttpQuery("openNums")),
3536
OpenNumsStr.schema.optional[Queries]("ons", _.ons).addHints(smithy.api.HttpQuery("openNumsStr")),
37+
double.validated(smithy.api.Range(min = Some(scala.math.BigDecimal(0.0)), max = Some(scala.math.BigDecimal(100.0)))).optional[Queries]("dbl", _.dbl).addHints(smithy.api.HttpQuery("dbl")),
3638
StringMap.underlyingSchema.optional[Queries]("slm", _.slm).addHints(smithy.api.HttpQueryParams()),
3739
)(make).withId(id).addHints(hints)
3840
}

‎modules/bootstrapped/test/src/smithy4s/DocumentSpec.scala

+22
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import munit._
2525
import smithy4s.example.DefaultNullsOperationOutput
2626
import alloy.Untagged
2727
import smithy4s.example.TimestampOperationInput
28+
import scala.util.Try
2829

2930
class DocumentSpec() extends FunSuite {
3031

@@ -370,6 +371,27 @@ class DocumentSpec() extends FunSuite {
370371
expect.same(roundTripped, Right(mapTest))
371372
}
372373

374+
test("encoding NaN") {
375+
// The Document type cannot hold a `NaN` value since it uses BigDecimal to hold numeric values
376+
// this test exists to show this. For the same reason, a test on decoding from `NaN` is not necessary
377+
// or possible.
378+
implicit val schema: Schema[Double] =
379+
double.validated(smithy.api.Range(None, Some(BigDecimal(3))))
380+
381+
val in = Double.NaN
382+
val error = Try(Document.encode(in)).failed.get
383+
val expectedMessage =
384+
if (weaver.Platform.isJS || weaver.Platform.isNative)
385+
"For input string: \"NaN\""
386+
else
387+
"Character N is neither a decimal digit number, decimal point, nor \"e\" notation exponential mark."
388+
389+
expect.same(
390+
error.getMessage,
391+
expectedMessage
392+
)
393+
}
394+
373395
test(
374396
"optional fields for structs should decode Document.DNull"
375397
) {

‎modules/bootstrapped/test/src/smithy4s/http/internals/MetadataSpec.scala

+43-1
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,28 @@ class MetadataSpec() extends FunSuite {
7575
.left
7676
.map(_.getMessage())
7777
expect.same(encoded, expectedEncoding)
78-
expect(result == Right(finished))
78+
expect.same(result, Right(finished))
79+
}
80+
81+
def checkQueryRoundTripError[A](
82+
initial: A,
83+
expectedEncoding: Metadata,
84+
errorMessage: String,
85+
allowNaN: Boolean
86+
)(implicit
87+
s: Schema[A],
88+
loc: Location
89+
): Unit = {
90+
val encoded = Metadata.encode(initial)
91+
val decoder =
92+
if (allowNaN) Metadata.AwsDecoder.fromSchema(s)
93+
else Metadata.Decoder.fromSchema(s)
94+
val result = decoder
95+
.decode(encoded)
96+
.left
97+
.map(_.getMessage())
98+
expect.same(encoded, expectedEncoding)
99+
expect.same(result, Left(errorMessage))
79100
}
80101

81102
def checkRoundTripDefault[A](expectedDecoded: A)(implicit
@@ -123,6 +144,27 @@ class MetadataSpec() extends FunSuite {
123144
checkQueryRoundTrip(queries, expected, finished)
124145
}
125146

147+
// In this test the Metadata Decoder will allow NaN by creating a `Double.NaN` value.
148+
// The Range RefinementProvider will reject this since `NaN` is not a valid `BigDecimal`
149+
// which it uses
150+
test("Double NaN query parameter - allow NaN in decoder") {
151+
val queries = Queries(dbl = Some(Double.NaN))
152+
val expected = Metadata(query = Map("dbl" -> List("NaN")))
153+
val errorMessage =
154+
"Field dbl, found in Query parameter dbl, failed constraint checks with message: Numeric values must not be NaN or pos/neg infinity. Found NaN"
155+
checkQueryRoundTripError(queries, expected, errorMessage, allowNaN = true)
156+
}
157+
158+
// This test is where the Metadata Decoder will reject NaN itself
159+
// As such the RefinementProvider for Range will not be called in this test
160+
test("Double NaN query parameter - disallow NaN in decoder") {
161+
val queries = Queries(dbl = Some(Double.NaN))
162+
val expected = Metadata(query = Map("dbl" -> List("NaN")))
163+
val errorMessage =
164+
"NaN or pos/neg infinity are not allowed for inputs of type Double"
165+
checkQueryRoundTripError(queries, expected, errorMessage, allowNaN = false)
166+
}
167+
126168
test("String query parameter with default") {
127169
val expectedDecoded = QueriesWithDefaults(dflt = "test")
128170
checkRoundTripDefault(expectedDecoded)

‎modules/core/src/smithy4s/RefinementProvider.scala

+28-21
Original file line numberDiff line numberDiff line change
@@ -161,27 +161,34 @@ object RefinementProvider extends LowPriorityImplicits {
161161
val N = implicitly[Numeric[N]]
162162

163163
(a: A) =>
164-
val value = BigDecimal(N.toDouble(getValue(a)))
165-
(range.min, range.max) match {
166-
case (Some(min), Some(max)) =>
167-
if (value >= min && value <= max) Right(())
168-
else
169-
Left(
170-
s"Input must be >= $min and <= $max, but was $value"
171-
)
172-
case (None, Some(max)) =>
173-
if (value <= max) Right(())
174-
else
175-
Left(
176-
s"Input must be <= $max, but was $value"
177-
)
178-
case (Some(min), None) =>
179-
if (value >= min) Right(())
180-
else
181-
Left(
182-
s"Input must be >= $min, but was $value"
183-
)
184-
case (None, None) => Right(())
164+
val doubleValue = N.toDouble(getValue(a))
165+
if (doubleValue.isNaN || doubleValue.isInfinite) {
166+
Left(
167+
s"Numeric values must not be NaN or pos/neg infinity. Found $doubleValue"
168+
)
169+
} else {
170+
val value = BigDecimal.apply(d = doubleValue)
171+
(range.min, range.max) match {
172+
case (Some(min), Some(max)) =>
173+
if (value >= min && value <= max) Right(())
174+
else
175+
Left(
176+
s"Input must be >= $min and <= $max, but was $value"
177+
)
178+
case (None, Some(max)) =>
179+
if (value <= max) Right(())
180+
else
181+
Left(
182+
s"Input must be <= $max, but was $value"
183+
)
184+
case (Some(min), None) =>
185+
if (value >= min) Right(())
186+
else
187+
Left(
188+
s"Input must be >= $min, but was $value"
189+
)
190+
case (None, None) => Right(())
191+
}
185192
}
186193
}
187194
}

‎modules/core/src/smithy4s/http/Metadata.scala

+18-5
Original file line numberDiff line numberDiff line change
@@ -171,15 +171,24 @@ object Metadata {
171171
implicit def decoderFromSchema[A: Schema]: Decoder[A] =
172172
Decoder.derivedImplicitInstance
173173

174-
object Decoder extends CachedDecoderCompilerImpl(awsHeaderEncoding = false) {
174+
object Decoder
175+
extends CachedDecoderCompilerImpl(
176+
awsHeaderEncoding = false,
177+
allowNaNAndInfiniteValues = false
178+
) {
175179
type Compiler = CachedSchemaCompiler[Decoder]
176180
}
177181

178182
private[smithy4s] object AwsDecoder
179-
extends CachedDecoderCompilerImpl(awsHeaderEncoding = true)
183+
extends CachedDecoderCompilerImpl(
184+
awsHeaderEncoding = true,
185+
allowNaNAndInfiniteValues = true
186+
)
180187

181-
private[http] class CachedDecoderCompilerImpl(awsHeaderEncoding: Boolean)
182-
extends CachedSchemaCompiler.DerivingImpl[Decoder] {
188+
private[http] class CachedDecoderCompilerImpl(
189+
awsHeaderEncoding: Boolean,
190+
allowNaNAndInfiniteValues: Boolean
191+
) extends CachedSchemaCompiler.DerivingImpl[Decoder] {
183192
type Aux[A] = internals.MetaDecode[A]
184193

185194
def apply[A](implicit instance: Decoder[A]): Decoder[A] =
@@ -190,7 +199,11 @@ object Metadata {
190199
cache: CompilationCache[internals.MetaDecode]
191200
): Decoder[A] = {
192201
val metaDecode =
193-
new SchemaVisitorMetadataReader(cache, awsHeaderEncoding)(schema)
202+
new SchemaVisitorMetadataReader(
203+
cache,
204+
awsHeaderEncoding,
205+
allowNaNAndInfiniteValues
206+
)(schema)
194207
metaDecode match {
195208
case internals.MetaDecode.StructureMetaDecode(decodeFunction) =>
196209
decodeFunction(_: Metadata)

‎modules/core/src/smithy4s/http/internals/SchemaVisitorMetadataReader.scala

+36-1
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,13 @@ import java.util.Base64
3737
* contains values such as path-parameters, query-parameters, headers, and status code.
3838
*
3939
* @param awsHeaderEncoding defines whether the AWS encoding of headers should be expected.
40+
* @param allowNaNAndInfiniteValues defines whether or not Double and Float values of 'NaN'
41+
* positive/negative infinity should be accepted.
4042
*/
4143
private[http] class SchemaVisitorMetadataReader(
4244
val cache: CompilationCache[MetaDecode],
43-
awsHeaderEncoding: Boolean
45+
awsHeaderEncoding: Boolean,
46+
allowNaNAndInfiniteValues: Boolean
4447
) extends SchemaVisitor.Cached[MetaDecode]
4548
with ScalaCompat { self =>
4649

@@ -50,6 +53,38 @@ private[http] class SchemaVisitorMetadataReader(
5053
tag: Primitive[P]
5154
): MetaDecode[P] = {
5255
val desc = SchemaDescription.primitive(shapeId, hints, tag)
56+
57+
tag match {
58+
case Primitive.PDouble =>
59+
val decode: MetaDecode[Double] =
60+
primitiveHandler(shapeId, hints, tag, desc)
61+
decode.map(d =>
62+
if (!allowNaNAndInfiniteValues && (d.isNaN || d.isInfinite))
63+
throw MetadataError.ImpossibleDecoding(
64+
s"NaN or pos/neg infinity are not allowed for inputs of type $desc"
65+
)
66+
else d
67+
)
68+
case Primitive.PFloat =>
69+
val decode: MetaDecode[Float] =
70+
primitiveHandler(shapeId, hints, tag, desc)
71+
decode.map(f =>
72+
if (!allowNaNAndInfiniteValues && (f.isNaN || f.isInfinite))
73+
throw MetadataError.ImpossibleDecoding(
74+
s"NaN or pos/neg infinity are not allowed for inputs of type $desc"
75+
)
76+
else f
77+
)
78+
case _ => primitiveHandler(shapeId, hints, tag, desc)
79+
}
80+
}
81+
82+
private def primitiveHandler[P](
83+
shapeId: ShapeId,
84+
hints: Hints,
85+
tag: Primitive[P],
86+
desc: String
87+
): MetaDecode[P] = {
5388
val hasMedia = hints.has(smithy.api.MediaType)
5489
Primitive.stringParser(tag, hints) match {
5590
case Some(parse) if hasMedia =>

‎modules/json/test/src/smithy4s/json/JsonSpec.scala

+11
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,17 @@ class JsonSpec() extends FunSuite {
4646
assertEquals(roundTripped, Right(foo))
4747
}
4848

49+
test("Json read - NaN") {
50+
implicit val schemaDouble: Schema[Double] =
51+
double.validated(smithy.api.Range(None, Some(BigDecimal(3))))
52+
val expectedJson = """"NaN""""
53+
val roundTripped = Json.read[Double](Blob(expectedJson))
54+
55+
assert(
56+
roundTripped.left.toOption.get.message.startsWith("illegal number")
57+
)
58+
}
59+
4960
test("Json document read/write") {
5061
val foo =
5162
Document.obj("a" -> Document.fromInt(1), "b" -> Document.fromInt(2))

‎sampleSpecs/metadata.smithy

+3
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,9 @@ structure Queries {
7272
on: OpenNums
7373
@httpQuery("openNumsStr")
7474
ons: OpenNumsStr
75+
@httpQuery("dbl")
76+
@range(min: 0, max: 100)
77+
dbl: Double
7578
@httpQueryParams
7679
slm: StringMap
7780
}

0 commit comments

Comments
 (0)