Skip to content

Commit

Permalink
no Integer in javascript, fixed Json
Browse files Browse the repository at this point in the history
  • Loading branch information
@sadache
sadache committed Mar 8, 2012
1 parent 6b87c78 commit 7f7c953
Show file tree
Hide file tree
Showing 4 changed files with 5 additions and 15 deletions.
1 change: 0 additions & 1 deletion framework/src/play/src/main/scala/play/api/data/Form.scala
View file
Expand Up @@ -327,7 +327,6 @@ private[data] object FormUtils {
case JsUndefined(_) => Map.empty
case JsBoolean(value) => Map(prefix -> value.toString)
case JsNumber(value) => Map(prefix -> value.toString)
case JsInteger(value) => Map(prefix -> value.toString)
case JsString(value) => Map(prefix -> value.toString)
}

Expand Down
10 changes: 2 additions & 8 deletions framework/src/play/src/main/scala/play/api/libs/json/JsValue.scala
View file
Expand Up @@ -76,11 +76,6 @@ case class JsBoolean(value: Boolean) extends JsValue
*/
case class JsNumber(value: BigDecimal) extends JsValue

/**
* Represent a Json integer number value.
*/
case class JsInteger(value: Long) extends JsValue

/**
* Represent a Json string value.
*/
Expand Down Expand Up @@ -175,8 +170,7 @@ private[json] class JsValueSerializer extends JsonSerializer[JsValue] {

def serialize(value: JsValue, json: JsonGenerator, provider: SerializerProvider) {
value match {
case JsNumber(v) => json.writeNumber(v.doubleValue())
case JsInteger(v) => json.writeNumber(v)
case JsNumber(v) => json.writeNumber(v.bigDecimal)
case JsString(v) => json.writeString(v)
case JsBoolean(v) => json.writeBoolean(v)
case JsArray(elements) => json.writeObject(elements)
Expand Down Expand Up @@ -239,7 +233,7 @@ private[json] class JsValueDeserializer(factory: TypeFactory, klass: Class[_]) e

val (maybeValue, nextContext) = (jp.getCurrentToken, parserContext) match {

case (JsonToken.VALUE_NUMBER_INT, c) => (Some(JsInteger(jp.getLongValue)), c)
case (JsonToken.VALUE_NUMBER_INT, c) => (Some(JsNumber(jp.getLongValue)), c)

case (JsonToken.VALUE_NUMBER_FLOAT, c) => (Some(JsNumber(jp.getDoubleValue)), c)

Expand Down
3 changes: 0 additions & 3 deletions framework/src/play/src/main/scala/play/api/libs/json/Reads.scala
View file
Expand Up @@ -35,7 +35,6 @@ trait DefaultReads {
implicit object IntReads extends Reads[Int] {
def reads(json: JsValue) = json match {
case JsNumber(n) => n.toInt
case JsInteger(n) => n.toInt
case _ => throw new RuntimeException("Int expected")
}
}
Expand All @@ -46,7 +45,6 @@ trait DefaultReads {
implicit object ShortReads extends Reads[Short] {
def reads(json: JsValue) = json match {
case JsNumber(n) => n.toShort
case JsInteger(n) => n.toShort
case _ => throw new RuntimeException("Short expected")
}
}
Expand All @@ -57,7 +55,6 @@ trait DefaultReads {
implicit object LongReads extends Reads[Long] {
def reads(json: JsValue) = json match {
case JsNumber(n) => n.toLong
case JsInteger(n) => n
case _ => throw new RuntimeException("Long expected")
}
}
Expand Down
6 changes: 3 additions & 3 deletions framework/src/play/src/main/scala/play/api/libs/json/Writes.scala
View file
Expand Up @@ -33,21 +33,21 @@ trait DefaultWrites {
* Serializer for Int types.
*/
implicit object IntWrites extends Writes[Int] {
def writes(o: Int) = JsInteger(o)
def writes(o: Int) = JsNumber(o)
}

/**
* Serializer for Short types.
*/
implicit object ShortWrites extends Writes[Short] {
def writes(o: Short) = JsInteger(o)
def writes(o: Short) = JsNumber(o)
}

/**
* Serializer for Long types.
*/
implicit object LongWrites extends Writes[Long] {
def writes(o: Long) = JsInteger(o)
def writes(o: Long) = JsNumber(o)
}

/**
Expand Down

7 comments on commit 7f7c953

@ikester
Copy link

@ikester ikester commented on 7f7c953 Mar 9, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that JSON only defines a "Number" but it's pretty annoying to send entity IDs as 9324.0. Do you have any suggestions to make APIs a little nicer, without all the unnecessary decimal points?

Why would it be a problem to keep the Integer serialization the way it was? Both would be valid JSON: 1 and 1.0.

@densh
Copy link

@densh densh commented on 7f7c953 Mar 9, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely agree with @ikester. In my experience non-js json libraries tend to treat numbers as two distinct types. For example python's json library deserializes json numbers into ints and floats.

@ikester
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadek, do you feel very strongly about this? How could it be harmful to have these additional Integer Writes?

@guillaumebort
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can't introduce JsInteger. It doesn't make sense at all. 9324.0 is as valid as 9324 in json since it has a single number type. And anyway since we switched to BigDecimal for the internal representation, this pseudo issue is now solved.

@ikester
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it is technically valid, but it's not very practical (or pretty) when you're trying to pass around numeric IDs, like primary keys (e.g. { id: 1243.0, name: "Some Name"}. I don't recall seeing any REST APIs that return integers like this.

I didn't understand what you meant by the BigDecimal pseudo issue.

I guess my real question is: why is it bad if we leave it in? Does it break something?

@guillaumebort
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that your issue should be fixed now. Writing 1243.0 as 1243 was just a printing issue. No need to introduce a new type in the user API for that.

@ikester
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh OK. I had missed that point. I just tested it and it printed the integer without decimals.

Thanks!

Please sign in to comment.