[FIX] Issue #122 / Add database/sql null type support #200
Conversation
… more manageable - three new functions were created: resolveNodeID(), resolveNodeAttribute() and resolveNodeRelation()
….NullInt32 and sql.NullInt64 type
- sql.NullBool - sql.NullString - sql.NullFloat64 - sql.NullInt32 - sql.NullInt64
… the sql null type isn't valid
|
Hi @aren55555, I would like your opinion on the work done so far. Cheers! |
response.go
Outdated
| case reflect.Uint64: | ||
| node.ID = strconv.FormatUint(v.Interface().(uint64), 10) | ||
| case reflect.Struct: | ||
| if nStr, ok := v.Interface().(sql.NullString); ok { |
There was a problem hiding this comment.
Other sql.Null* types don't make sense for an ID, so we only consider: sql.NullString, sql.NullInt32 and sql.NullInt64.
| node.Attributes[args[1]] = t.Unix() | ||
| } | ||
| } | ||
| case reflect.TypeOf(sql.NullTime{}): |
There was a problem hiding this comment.
I'm not sure how common the usage of *sql.NullTime is (if that even makes sense), so for now I just added support for sql.NullTime.
response.go
Outdated
|
|
||
| if node.Relationships == nil { | ||
| node.Relationships = make(map[string]interface{}) | ||
| if nI32, ok := fieldValue.Interface().(sql.NullInt32); ok { |
There was a problem hiding this comment.
I couldn't find a DRY way to handle the repetitive checks sql.Null* types require (omit attribute, return null and return value).
| node, err = resolveNodeID(node, fieldValue, fieldType) | ||
|
|
||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Returning early, instead of breaking and dealing with the error further down.
|
Please add tests that verify both the marshaling and unmarshalling behavior of these new types. |
…wn functions - unmarshallID() - unmarshallRelation()
…dleNumeric() function
…() function - implement isSQLNullType() and handleSQLNullType() functions
|
|
||
| json.NewEncoder(buf).Encode(data.Relationships[args[1]]) | ||
| json.NewDecoder(buf).Decode(relationship) | ||
| func unmarshallID(node *Node, fieldValue reflect.Value, structField reflect.StructField) (*Node, error) { |
There was a problem hiding this comment.
unmarshallID isn't the greatest name, so I'm open to suggestions.
|
|
||
| func unmarshallRelation(node *Node, fieldValue reflect.Value, included *map[string]*Node, args []string) (*Node, error) { |
There was a problem hiding this comment.
Same here, feel free to suggest a better function name.
| if data.Attributes["decimal"] != 3.0 { | ||
| t.Fatalf("Error marshalling to sql.NullInt32") | ||
| } | ||
|
|
There was a problem hiding this comment.
Unlike in the previous test, I'm not checking the expected values of data.Attributes["fractional"] and data.Attributes["computed_at"], since what we're getting doesn't make much sense to me.
I would expect data.Attributes["fractional"] to be 1415926535897932, yet we get 1.415926535897932e+15.
The same goes for data.Attributes["computed_at"], where we get 1.615734e+09 instead of 1615734000.
I'm not sure if this is expected, but it doesn't feel right. I also wonder if this is somehow related to #202, where we get a different float64 when converting values back.
TL;DR; I'm expecting different numeric types (int32 and int64), but get float64 instead.
…re invalid and the omitted tag isn't set
…itempty is not set
|
Nice feature. Would be great if you merge it into the master. |
Yeah, but we need #206 merged first. Ping the repository owner if you have an interest. |
Description
This pull request addresses issue #122, which aims to add support for
database/sqlnull types.I mainly want to start a discussion about the implementation, since there's definitely room for change/improvement.
Also, while pushing my changes, I realised from the TravisCI results that Go versions prior to 1.13 don't have support for
database/sqlnull types. Would bumping the minimum Go version to 1.13 be a deal breaker?Task items:
visitModelNode()function logic, to be more manageable/readable;resolveNodeID();sql.NullString,sql.NullInt32,sql.NullInt64andsql.NullFloat64resolveNodeAttribute();sql.NullTime,sql.NullBool,sql.NullString,sql.NullFloat64,sql.NullInt32andsql.NullInt64resolveNodeRelation();unmarshalNode()function logic, to be more manageable/readable;unmarshallID();sql.NullString,sql.NullInt32,sql.NullInt64andsql.NullFloat64sql.NullTime,sql.NullBool,sql.NullString,sql.NullFloat64,sql.NullInt32andsql.NullInt64unmarshallRelation();