database/sql: panic inserting a nil pointer when underlying type implements driver.Valuer

[a13xb]

*What does 'go version' print?*

go version go1.3 darwin/amd64

*What steps reproduce the problem?*

See attached sample. It needs Postgres to run, but another DB should work just as well,
there shouldn't be driver-dependent code involved.

*What happened?*

Panic in case 6 in the sample program.

{{{
panic: value method main.MyType.Value called using nil *MyType pointer

goroutine 16 [running]:
runtime.panic(0x1c1980, 0xc208000960)
    /usr/local/Cellar/go/1.3/libexec/src/pkg/runtime/panic.c:279 +0xf5
main.(*MyType).Value(0x0, 0x0, 0x0, 0x0, 0x0)
    <autogenerated>:1 +0x97
database/sql/driver.defaultConverter.ConvertValue(0x2053a0, 0x0, 0x0, 0x0, 0x0, 0x0)
    /usr/local/Cellar/go/1.3/libexec/src/pkg/database/sql/driver/types.go:219 +0x101
database/sql.driverArgs(0x0, 0xc208095f20, 0x1, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0)
    /usr/local/Cellar/go/1.3/libexec/src/pkg/database/sql/convert.go:35 +0x1e1
database/sql.(*DB).exec(0xc208054100, 0x28a1f0, 0x1b, 0xc208095f20, 0x1, 0x1, 0x0, 0x0,
0x0, 0x0)
    /usr/local/Cellar/go/1.3/libexec/src/pkg/database/sql/sql.go:884 +0x1ec
database/sql.(*DB).Exec(0xc208054100, 0x28a1f0, 0x1b, 0xc208095f20, 0x1, 0x1, 0x0, 0x0,
0x0, 0x0)
    /usr/local/Cellar/go/1.3/libexec/src/pkg/database/sql/sql.go:866 +0xd4
main.main()
    /Users/alex/g/main.go:60 +0x6b5
}}}

*What should have happened instead?*

Behaviour of Exec() when handling nil pointers of values that implement sql.Valuer
should be consistent with primitive values. I can insert a pointer to string, and if
it's nil, it will translate to SQL NULL just fine.

Attachments:

1. main.go (996 bytes)

[griesemer]

Comment 1:

Labels changed: added repo-main.

[rsc]

Even attempting to do this would require package reflect in database/sql, which is a step backward. If you want to use *MyType with this package, then put the Valuer method on *MyType.

[a13xb]

@rsc

Well... database/sql/driver already uses reflect package. In fact, in the very same function that cause the panic (defaultConverter.ConvertValue), so I'm not sure what you are implying.

Also, here is another related problem I've run into:

When I implement Valuer on *MyType instead of MyType, it (obviously) stops working on on non-pointer values. Because you cannot use MyType and *MyType as Valuer at the same time, you cannot have a nullable Valuer fields in frameworks like gorp, where I don't directly control per-field binding code. Well, theoretically you can, but then they all have to be pointers, even non-nullable fields, which is a nuisance, and is not consistent with basic types. E.g. I can have a mix of time.Time and *time.Time fields, but not MyType and *MyType columns.

So my current workaround is to only use standard types in database code, and avoid any uses of Valuer.

[danilobuerger]

@rsc why is this issue closed? @a13xb makes an excellent point. If its implemented on *MyType i can no longer use MyType.

[ianlancetaylor]

Yes, I think @rsc missed something here. The database/sql/driver package already relies on the reflect pkacage. If the value implements Valuer, we need to check whether it is a pointer, whether it is nil, and whether the Value method is implemented on a value rather than on a pointer. This all seems doable. Maybe something like

if rv.Kind() == reflect.Ptr && rv.IsNil && rv.Type().Elem().Implements(reflect.TypeOf(Valuer)) {
    return nil, nil
}

Reopening in case somebody wants to implement this, with a test.

[ConnorFoody]

@ianlancetaylor I think you may have it backwards in your snippet.

It seems like you want the code to check if a pointer to the var takes the Value interface, then try to get that pointer. I believe all the other combos of implemented on a pointer or not / value a pointer or not work with the existing code.

I got this working, with tests, on my machine:

ptrType := reflect.PtrTo(reflect.TypeOf(v))
if ptrType.Implements(reflect.TypeOf((*Valuer)(nil)).Elem()) {

    uPtr := unsafe.Pointer(&v)
    tPtr := reflect.NewAt(ptrType, uPtr)

    return defaultConverter{}.ConvertValue(tPtr.Elem().Interface())
}

I had to use unsafe to create the pointer because it wouldn't let let me do something like:

ptr, ok := (interface{}(&val)).(Valuer)

However, I am a go noobie so I may be missing something. What do you think?

[ianlancetaylor]

@ConnorFoody OK, that is a different perspective. You are suggesting that people should always write a pointer method, and the database/sql package should invent a pointer if necessary.

[ConnorFoody]

@ianlancetaylor Thanks for getting back to me! Sorry, I didn't properly read the issue. I agree with what you said earlier. The call on the nil value happens in a loop that builds and returns an array so I think the proper solution is to add the value into the array.

I changed the code to check if the value is nil between the cast to a driver.Valuer and the call to Value:

if svi, ok := arg.(driver.Valuer); ok {
  svk := reflect.TypeOf(svi).Kind()
  canCheckNil := svk == reflect.Interface || svk == reflect.Ptr
  if canCheckNil && reflect.ValueOf(arg).IsNil() {
    arg = svi // new code
    //arg = nil <-- can't do this
  } else {
    // do current approach
    sv, err := svi.Value()

For whatever reason it wouldn't take a nil value. It seems like it is indented behaviour for it to not take a nil value, but it sort of seems odd that it would take a typed nil but not a straight up nil.

[ConnorFoody]

@ianlancetaylor Sorry for the bump, would appreciate your help in getting this through.

The test looks like this :

var ptr *nonPtrImpl
ptr = nil
...
stmt.Exec(ptr, ...) == _, "" // used to panic

And the fix (line for context) looks like this:

if svi, ok := arg.(driver.Valuer); ok {
 svk := reflect.TypeOf(svi).Kind()
  canCheckNil := svk == reflect.Interface || svk == reflect.Ptr
  if canCheckNil && reflect.ValueOf(arg).IsNil() {
    arg = svi // new code
  } else {
    ...
    arg = svi.Value() // where panic was happening
  }
  // convert arg

All the tests passed with all.bash. Does this look ok to you? May I submit for review?

[ianlancetaylor]

First I should say that I really don't know much about the database/sql package.

Your suggested change does not seem quite right to me. Your code seems to go back to the case I was discussing when I reopened the issue above.

You don't need to check for whether Kind() == Interface. In that case, a nil value won't implement driver.Valuer anyhow.

If the pointer is nil, but driver.Valuer is implemented on the pointer type, we should call the method. It's fine to call a method on a nil pointer. The Value method can decide what to do.

The only interesting case is a pointer that is nil for which the Value method is implemented on a value. In that case we can't call the method, so we should just stick the nil. That is what I was suggesting above.

[220kts]

We ran into the same issue. In function ConvertValue (sql/driver/types.go) could we not implement the same check for nil pointers that is used on standard types (line 238) on types that implement the Valuer interface?

Something like:

if svi, ok := v.(Valuer); ok {

        rv := reflect.ValueOf(svi)
        if rv.Kind() == reflect.Ptr && rv.IsNil() {
                return nil, nil
        }

        sv, err := svi.Value()
        if err != nil {
            return nil, err
        }
        if !IsValue(sv) {
            return nil, fmt.Errorf("non-Value type %T returned from Value", sv)
        }
        return sv, nil
    }

To be clear, this would always insert NULL for a nil pointer, regardless of how the underlying type's Value method handles nil pointers, in cases where the Value method is defined on a pointer. We could check if the method is implemented on a pointer and then call it, however, I can't imagine any situation where you would not want to insert a dbase NULL for a nil pointer.

[blockloop]

I just want to share a temporary solution to this problem. My database uses a nullable UUID for a column. I'm using github.com/twinj/uuid. I've created a wrapper called NullableUUID and instead of using uuid.UUID as the struct data types I'm using NullableUUID.

// NullableUUID wrapper to fix nullable UUID. See https://github.com/golang/go/issues/8415
type NullableUUID uuid.Uuid

// Value implements Sql/Value so it can be converted to DB value
func (u *NullableUUID) Value() (driver.Value, error) {
    if u == nil || len(*u) == 0 {
        return nil, nil
    }
    return u.MarshalText()
}

// MarshalText helps convert to value for JSON
func (u *NullableUUID) MarshalText() ([]byte, error) {
    if u == nil || len(*u) == 0 {
        return nil, nil
    }
    return uuid.Uuid(*u).MarshalText()
}

// UnmarshalText helps convert to value for JSON
func (u *NullableUUID) UnmarshalText(data []byte) error {
    if len(data) == 0 {
        return nil
    }

    parsed, err := uuid.Parse(string(data))
    if err != nil {
        return err
    }

    *u = NullableUUID(parsed)
    return nil
}


type opportunity struct {
    ID                  *int          `json:"id,omitempty" db:"OpportunityId,omitempty"`
    GlobalOpportunityID *NullableUUID `json:"globalOpportunityId,string,omitempty" db:"GlobalOpportunityId,omitempty"`
    GlobalPartnerID     *NullableUUID `json:"globalPartnerId,string,omitempty" db:"GlobalPartnerId,omitempty"`
}

[blockloop]

Does @220kts have the right solution?

[kardianos]

The following code would work to fix this issue: https://go-review.googlesource.com/30814 .

I'm uncertain if this is the correct fix (feedback welcome). While it will resolve this issue, the issue may also be resolved by documenting that the driver.Valuer interface should not contain nil, but just return a nil value.

[220kts]

Your fix looks good to me. IMO documenting that the driver.Valuer interface should not contain nil is problematic on the client side.

Thanks.

[kardianos]

@220kts Could you explain with some examples why that is problematic for a consumer of the database/sql package?

[220kts]

We use the Valuer and Scanner interfaces frequently for JSON data from the internet that we insert/retrieve to a json or jsonb column in Postgres but where we want to decode the JSON representation to a Go type first (ie. not use a []byte). We may want to decode the JSON to Go types to do something with the data or just validate that the specific JSON representation sent to the dbase has the properties we need. For any such type, there may be instances where the data is optional (database column accepts NULL) and other instances where the data is required (database column has a not null constraint).

In a non-trivial application, the most elegant and robust client implementation using those types is to use pointers for optional data and values for required data, this corresponds logically to the dbase structure and ensures the output to the internet corresponds to the data stored in the dbase. However, doing so requires implementing the Value() method on a value, which currently breaks when the Value() method is called on a nil pointer.

If my client has to enforce that the driver.Valuer interface is not nil then the client would have to invent an entirely new scheme to send null out to the internet on output, since I'm now working with not-nil variables where my underlying data in the dbase is actually NULL.

The attached is an abbreviated example of a large-ish project around building codes for HVAC systems. We need to have design conditions (climate data) for each home in our dbase but there are optional design conditions for each HVAC system that would locally overwrite the project's design conditions (say you wanted your inlaw apartment heated to 75F instead of 70F for the rest of the house).

Let me know if any questions or issues.

8514.zip

The issue is exactly as @ianlancetaylor stated previously:

The only interesting case is a pointer that is nil for which the Value method is implemented on a value. In that case we can't call the method, so we should just stick the nil.

[kardianos]

The other option for the HVAC example is to do the following:

type designConditions struct {
    NULL bool `json:-`
    Indoor      *indoorDesignConditions     `json:"indoor"`
    Outdoor     *outdoorDesignConditions    `json:"outdoor"`
}

func (m *designConditions) Scan(value interface{}) error {
    if value == nil {
        m.NULL = true
        return nil
    }
    data := value.([]byte)
    return  json.Unmarshal(data, m)
}

func (m designConditions) Value() (driver.Value, error) {
    if m.NULL {
        return nil, nil
    }
    return json.Marshal(m)
}

Do you see a problem with using the above design? Rather than passing in a null *designConditions, you would pass in a designConditions{NULL: true}.

I'm not suggesting you alter a schema design to not include nulls, I'm just suggesting there might be a more appropriate way to design the custom Valuer type. Would that work for you? Would it be an issue in your code?

[220kts]

@kardianos I understand we can implement a workaround like that (for example, the pq NullTime type works in a similar fashion) but it's not ideal: if I add the NULL property you are suggesting then I have to write custom JSON Marshal and Unmarshal functions or my JSON output to the client will be some type of empty JSON object instead of a JavaScript null. This can be done of course but it adds unnecessary code and tends to obfuscate your application's logic, ie. when a piece of data is NULL in the datastore the most logical thing is to have corresponding nil in the application.

Perhaps I'm missing something but what is the argument not to make this fix?

Thanks

[220kts]

@kardianos It's also worth noting that the current implementation is different between basic types and types that implement the Valuer interface and inconsistent with existing documentation:

If the argument is a nil pointer, ConvertValue returns a nil Value.

Currently, for a nil pointer on a basic type, ConvertValue returns nil, for a nil pointer on a type that implements the Valuer interface, it calls the Value() method if implemented on a pointer or breaks if the Value() method is implemented on a value. It doesn't seem very elegant to me to change the documentation to state that the client can send nil pointers on basic types but not on types that implement the Valuer interface?

On the other hand the code you wrote for the proposed fix brings the behavior inline with what is currently documented.

[kardianos]

@220kts

Perhaps I'm missing something but what is the argument not to make this fix?

A nil valued interface is always problematic and is generally avoided. I also have small concerns of the time cost it will add for this check for an edge case that can be avoided by construction the type more idiomatically.

I'm going to remove the DO NOT SUMBIT label and send to @bradfitz . I'm not super keen on it, but it does fit into the general framework database/sql uses.

[gopherbot]

CL https://golang.org/cl/30814 mentions this issue.

[kardianos]

@220kts
Here is how the code should read:

type designConditions struct {.
    Indoor      *indoorDesignConditions     `json:"indoor"`
    Outdoor     *outdoorDesignConditions    `json:"outdoor"`
}

func (m *designConditions) Scan(value interface{}) error {
    data := value.([]byte)
    return  json.Unmarshal(data, m)
}

func (m *designConditions) Value() (driver.Value, error) {
    if m == nil {
        return nil, nil
    }
    return json.Marshal(m)
}

Key change is Value is now a pointer to designConditions, not value, and it checks if m is nil before marshal.

This issue should be closed. This isn't an issue with database/sql.

[220kts]

@bradfitz @kardianos

I disagree but at the end of the day I can live with it. My main concerns are this:

1. The client doesn't always control NULL vs not NULL on a per column basis. The existing database/sql implementation makes writing client side code using the Valuer interface problematic. For example, the code @kardianos is suggesting above would cause a break in my project.go file. I find the other options to make the Valuer interface work on both NULL and not NULL columns a bit hack-ish (like adding an IsNull property in the type).

2. Implementing the Value() function with a value receiver is not user error per se. Nothing in the application will ever call that function, only the database/sql package.

For what it's worth, the person consuming the database/sql package is likely an average application programmer like myself and they - for better or worse - expect it to "just work". With all due respect, if @bradfitz says "that bug is confusing" you can be sure it is confusing for the average programmer consuming the database/sql package.

Our workaround has been to implement the Value() function with a pointer receiver, remove the not NULL constraints on all corresponding columns and (obviously) use pointers for all our client side variables. That's the only way we can be reasonably sure not to cause a conflict at the dbase since our client side applications are of significant enough size that we could never guarantee none of our pointers will be nil.

[bradfitz]

Why is column's nullability in the database schema a factor in this discussion? If the column is NOT NULL and you send a NULL on accident, the database will return an error. That's good, no? It helps you find your mistakes.

When I say this bug is confusing, it's because it's long, bounces around different topics, and doesn't have a concise summary anywhere. I admit I haven't read the whole thread.

@kardianos, care to summarize? Maybe there's something to do here with better error messages at least?

[blockloop]

I don't know how columns nullability got involved, but the reason I followed this discussion is represented in the OP. Given the following code

type Person struct {
    ID *uuid.UUID
}

When uuid.UUID is a Valuer the database/sql codebase tries to call .Value() even when the pointer is nil which causes the error in the OP: "panic: value method main.Person.Value called using nil *Person pointer." Given a nil pointer the database/sql package should try to insert NULL into the database column. If the database has an issue with that then that error can be raised separately.

I guess what I'm saying is that the database/sql package should not rely on the result of Value() if the pointer is nil. If the pointer is nil, then assume the database value is NULL. Is that a bad assumption? If so, how?

I understand that someone may want to cover these problems with their codebase, but isn't there a better way to handle that? If I wanted to substitute *uuid.UUID with some default value (i.e. not NULL) then I could do that before calling the database/sql package and not within the Value() method. Also, if the database doesn't allow nulls then why use a pointer as the struct property? Why not use uuid.UUID rather than *uuid.UUID and then you don't have to worry about getting a NULL?

[220kts]

@bradfitz In my experience column's nullability typically determines whether we use a value or a pointer on the application's side. That's not a hard requirement but it makes logical sense and helps to ensure that our application's output corresponds to the data in the datastore.

In my opinion the essence of this bug/feature is simply that: it allows the client to use a mix of values and pointers for types that implement the Valuer interface.

[a13xb]

When I say this bug is confusing, it's because it's long, bounces around different topics, and doesn't have a concise summary anywhere. I admit I haven't read the whole thread.

@bradfitz The original code sample submitted gives a good summary. The main issue is consistency of handling builtin types and Valuer types.

[kardianos]

@bradfitz The posters are creating a MyType that implements the driver.Valuer interface. They have an existing practice in their applications of representing nullable columns as *(type) and not null columns as (type). The issue they are running into is that when they have a *MyType driver.Valuer it fails because the implementation func (mt MyType) Value() (driver.Value, error) is implemented on the value type rather than the pointer value type. They define the Value function on the non-pointer value because then they can use the type with both their "nullable" types and "not null" types, because they made the choice to represent nullable types with *(type).

Of course the above doesn't mesh with how Go operates, as it makes a situation where the valuer type is of type MyType so the Valuer interface is not nil but points to nothing and the Valuer method cannot be used as it isn't called in this case.

---

I see two solutions that doesn't involve modifying database/sql:

• Use a type that encodes NULL internally (with a bool for example) rather than on the type itself.
• Define type Value method on the pointer value type and always use the type consistently, regardless of the database type definition.

I understand both of these solutions would alter how you handle NULL values in your existing programs, or at least how you would prefer to handle them.

---

As a personal opinion, I think it is a mistake to construe Go's nil with the database NULL concept. There is a great talk about how the nil value is still useful and is distinct from database NULL you may have seen https://speakerdeck.com/campoy/understanding-nil https://www.youtube.com/watch?v=ynoY2xz-F8s .

[blockloop]

I agree with @a13xb the package converts a nil string pointer to database NULL and I expected it to do the same for non builtins.

[bradfitz]

I agree with @a13xb the package converts a nil string pointer to database NULL and I expected it to do the same for non builtins.

Okay, that's a pretty good argument.

@kardianos, how about https://golang.org/cl/31259 ?

[gopherbot]

CL https://golang.org/cl/31259 mentions this issue.

[blockloop]

Was this merged in 1.8? I didn't see it in the release notes.

[rsc]

Yes. If you click on the hash 0ce1d79 above, that page will show, just under the commit message text, the tags that contain this commit. go1.8 is there.

The release notes do not list every fixed bug.

[blockloop]

Thank you very much