Skip to content

Commit

Permalink
*Breaking* types change: JsonText -> JSONText, also document the []by…
Browse files Browse the repository at this point in the history
…te dest memory corruption bug and discourage JSONText and GzippedText both in package documentation and in the readme
  • Loading branch information
jmoiron committed Jan 12, 2016
1 parent 60a229c commit 4301d72
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 34 deletions.
25 changes: 7 additions & 18 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,26 +20,15 @@ explains how to use `database/sql` along with sqlx.

## Recent Changes

The addition of `sqlx.In` and `sqlx.Named`, which can be used to bind IN style
queries and named queries respectively.
* sqlx/types.JsonText has been renamed to JSONText to follow Go naming conventions.

```go
ids := []int{1, 2, 3}
query, args, err := sqlx.In("SELECT * FROM person WHERE id IN(?);", ids)

chris := Person{First: "Christian", Last: "Cullen"}
query, args, err := sqlx.Named("INSERT INTO person VALUES (:first, :last);", chris)
This breaks backwards compatibility, but it's in a way that is trivially fixable
(`s/JsonText/JSONText/g`). The `types` package is both experimental and not in
active development currently.

// these can be combined:
arg := map[string]interface{}{
"published": true,
"authors": []{8, 19, 32, 44},
}
query, args, err := sqlx.Named("SELECT * FROM articles WHERE published=:published AND author_id IN (:authors)", arg)
query, args, err := sqlx.In(query, args...)
// finally, if you're using eg. pg, you can rebind the query:
query = db.Rebind(query)
```
More importantly, [golang bug #13905](https://github.com/golang/go/issues/13905)
makes `types.JSONText` and `types.GzippedText` _potentially unsafe_, **especially**
when used with common auto-scan sqlx idioms like `Select` and `Get`.

### Backwards Compatibility

Expand Down
34 changes: 21 additions & 13 deletions types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ import (

// GzippedText is a []byte which transparently gzips data being submitted to
// a database and ungzips data being Scanned from a database.
// WARNING: due to go issue https://github.com/golang/go/issues/13905 it is
// potentially unsafe to use any []byte alias (like GzippedText) unless you
// use it as if it had the behavior of sql.RawBytes (ie. it ceases to be
// valid at the next scan)
type GzippedText []byte

// Value implements the driver.Valuer interface, gzipping the raw value of
Expand Down Expand Up @@ -48,21 +52,25 @@ func (g *GzippedText) Scan(src interface{}) error {
return nil
}

// JsonText is a json.RawMessage, which is a []byte underneath.
// JSONText is a json.RawMessage, which is a []byte underneath.
// Value() validates the json format in the source, and returns an error if
// the json is not valid. Scan does no validation. JsonText additionally
// the json is not valid. Scan does no validation. JSONText additionally
// implements `Unmarshal`, which unmarshals the json within to an interface{}
type JsonText json.RawMessage
// WARNING: due to go issue https://github.com/golang/go/issues/13905 it is
// potentially unsafe to use any []byte alias (like JSONText) unless you
// use it as if it had the behavior of sql.RawBytes (ie. it ceases to be
// valid at the next scan)
type JSONText json.RawMessage

// MarshalJSON returns the *j as the JSON encoding of j.
func (j *JsonText) MarshalJSON() ([]byte, error) {
func (j *JSONText) MarshalJSON() ([]byte, error) {
return *j, nil
}

// UnmarshalJSON sets *j to a copy of data
func (j *JsonText) UnmarshalJSON(data []byte) error {
func (j *JSONText) UnmarshalJSON(data []byte) error {
if j == nil {
return errors.New("JsonText: UnmarshalJSON on nil pointer")
return errors.New("JSONText: UnmarshalJSON on nil pointer")
}
*j = append((*j)[0:0], data...)
return nil
Expand All @@ -71,7 +79,7 @@ func (j *JsonText) UnmarshalJSON(data []byte) error {

// Value returns j as a value. This does a validating unmarshal into another
// RawMessage. If j is invalid json, it returns an error.
func (j JsonText) Value() (driver.Value, error) {
func (j JSONText) Value() (driver.Value, error) {
var m json.RawMessage
var err = j.Unmarshal(&m)
if err != nil {
Expand All @@ -81,26 +89,26 @@ func (j JsonText) Value() (driver.Value, error) {
}

// Scan stores the src in *j. No validation is done.
func (j *JsonText) Scan(src interface{}) error {
func (j *JSONText) Scan(src interface{}) error {
var source []byte
switch src.(type) {
case string:
source = []byte(src.(string))
case []byte:
source = src.([]byte)
default:
return errors.New("Incompatible type for JsonText")
return errors.New("Incompatible type for JSONText")
}
*j = JsonText(append((*j)[0:0], source...))
*j = JSONText(append((*j)[0:0], source...))
return nil
}

// Unmarshal unmarshal's the json in j to v, as in json.Unmarshal.
func (j *JsonText) Unmarshal(v interface{}) error {
func (j *JSONText) Unmarshal(v interface{}) error {
return json.Unmarshal([]byte(*j), v)
}

// Pretty printing for JsonText types
func (j JsonText) String() string {
// Pretty printing for JSONText types
func (j JSONText) String() string {
return string(j)
}
6 changes: 3 additions & 3 deletions types/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ func TestGzipText(t *testing.T) {
}
}

func TestJsonText(t *testing.T) {
j := JsonText(`{"foo": 1, "bar": 2}`)
func TestJSONText(t *testing.T) {
j := JSONText(`{"foo": 1, "bar": 2}`)
v, err := j.Value()
if err != nil {
t.Errorf("Was not expecting an error")
Expand All @@ -34,7 +34,7 @@ func TestJsonText(t *testing.T) {
t.Errorf("Expected valid json but got some garbage instead? %#v", m)
}

j = JsonText(`{"foo": 1, invalid, false}`)
j = JSONText(`{"foo": 1, invalid, false}`)
v, err = j.Value()
if err == nil {
t.Errorf("Was expecting invalid json to fail!")
Expand Down

0 comments on commit 4301d72

Please sign in to comment.