Use norman:"pointer" for nullable fields #400

cmurphy · 2021-05-19T21:18:46Z

Add a new field attribute "pointer" to indicate that the generated
client code for the field must be a pointer. This allows clients to
differentiate between sending nil/leaving the value unset and sending an
empty map or slice.

This change also removes the nullablestring norman type introduced in
30f8d18 since schemas that need a pointer to a string can now use this
field attribute. There are no libraries currently using this feature so
it should be safe to remove.

Example usage:

Labels map[string]string `json:"labels" norman:"pointer"`

Resulting API schema:

"labels": {
  "create": true,
  "nullable": true,
  "pointer": true,
  "type": "map[string]",
  "update": true
}

Generated client code:

Labels *map[string]string `json:"labels,omitempty" yaml:"labels,omitempty"`

rancher/rancher#32553

cmurphy · 2021-05-19T21:20:44Z

This is my other idea for fixing the issue we're having in the KEv2 operators: cmurphy@c685c62 - it's more direct about the problem, but I don't feel great about the extra attribute in the schema when it only has meaning in the generated client code.

cmurphy · 2021-05-19T23:06:37Z

@ibuildthecloud thoughts on this?

thedadams · 2021-05-26T00:53:18Z

I like this solution. I think it create a little more cumbersome client behavior, but I don't really see another way around the nil/empty issue without writing custom unmarshalers.

mjura

LGTM

thedadams · 2021-05-26T16:20:56Z

generator/generator.go

@@ -46,6 +46,8 @@ func getTypeString(nullable bool, typeName string, schema *types.Schema, schemas
 		return "map[string]" + getTypeString(false, typeName[len("map["):len(typeName)-1], schema, schemas)
 	case strings.HasPrefix(typeName, "array["):
 		return "[]" + getTypeString(false, typeName[len("array["):len(typeName)-1], schema, schemas)
+	case strings.HasPrefix(typeName, "nullable"):
+		return "*" + getTypeString(false, typeName[len("nullable"):len(typeName)], schema, schemas)


Suggested change

return "*" + getTypeString(false, typeName[len("nullable"):len(typeName)], schema, schemas)

return "*" + getTypeString(false, strings.TrimPrefix(typeName, "nullable"), schema, schemas)

rmweir

I did a PR for a similar issue I believe: https://github.com/rancher/norman/pull/382/files. It looks like maps could use the same fix in order to preserve the nil value. By reading the description for this PR it sounds similar but when reading the original issue it sounds like the problem is that omitting a value makes it convert to nil? Could I get some clarification on the issue?

cmurphy · 2021-06-01T19:49:04Z

It looks like maps could use the same fix in order to preserve the nil value.

@rmweir The problem happens for arrays too. That case is not getting hit because the JSON blob in the request is not just setting those fields to nil, it's omitting them entirely. So when the map is iterated over here, it never hits e.g. the locations field because it was never sent, because it was set to an empty slice.

By reading the description for this PR it sounds similar but when reading the original issue it sounds like the problem is that omitting a value makes it convert to nil? Could I get some clarification on the issue?

It's similar but it's a different layer in the parsing. Those array and map fields never run through the conversion because they're not even there. The result of that is the spec object gets created with those fields set to nil.

rmweir

This is probably fine. Ideally we wouldn't have to say it's a map pointer but this fix has a lot less potential to create undesirable side effects in other places so I think it is worth it. Was going to say to leave a comment but by the time someone get's to this logic it will probably be self explanatory. I worry a bit about it being generalized, but null int and similar types are already supported right?

rmweir · 2021-06-02T18:58:51Z

types/definition/definition.go

+	return (strings.HasPrefix(fieldType, "map[") || strings.HasPrefix(fieldType, "nullablemap[")) && strings.HasSuffix(fieldType, "]")
 }

 func IsArrayType(fieldType string) bool {
-	return strings.HasPrefix(fieldType, "array[") && strings.HasSuffix(fieldType, "]")
+	return (strings.HasPrefix(fieldType, "array[") || strings.HasPrefix(fieldType, "nullablearray[")) && strings.HasSuffix(fieldType, "]")


It looks like the nullable logic applies to any type now? Should this logic be added to IsReferenceType in that case?

I guess it should, for the sake of completeness. I'm not 100% sure what the implications would be for generalizing it to the reference[] type. It doesn't exactly map to a regular Go type like slices and maps, I can't picture a case where it would make sense to use a nullablereference[]. But it has to be opted into so it shouldn't hurt anything.

Maybe if a field is optional but otherwise should reference an existing type? If we don't intend for it to be used that way let's just remove it; removing it should cause an error to be returned when the type can't be parsed. We can always add the functionality if we end up wanting it. My bad.

cmurphy

I worry a bit about it being generalized, but null int and similar types are already supported right?

These already fall into the regular case of being nullable primitives so there's no need to explicitly use a nullableint type:

norman/generator/generator.go

Lines 58 to 63 in e48df26

    
           case "boolean": 
        
           	name = "bool" 
        
           case "float": 
        
           	name = "float64" 
        
           case "int": 
        
           	name = "int64"

norman/generator/generator.go

Lines 99 to 101 in e48df26

    
           if nullable { 
        
           	return "*" + name 
        
           }

cmurphy · 2021-06-03T00:49:35Z

types/definition/definition.go

+	return (strings.HasPrefix(fieldType, "map[") || strings.HasPrefix(fieldType, "nullablemap[")) && strings.HasSuffix(fieldType, "]")
 }

 func IsArrayType(fieldType string) bool {
-	return strings.HasPrefix(fieldType, "array[") && strings.HasSuffix(fieldType, "]")
+	return (strings.HasPrefix(fieldType, "array[") || strings.HasPrefix(fieldType, "nullablearray[")) && strings.HasSuffix(fieldType, "]")


I guess it should, for the sake of completeness. I'm not 100% sure what the implications would be for generalizing it to the reference[] type. It doesn't exactly map to a regular Go type like slices and maps, I can't picture a case where it would make sense to use a nullablereference[]. But it has to be opted into so it shouldn't hurt anything.

rmweir · 2021-06-04T04:18:39Z

types/definition/definition.go

+	return (strings.HasPrefix(fieldType, "map[") || strings.HasPrefix(fieldType, "nullablemap[")) && strings.HasSuffix(fieldType, "]")
 }

 func IsArrayType(fieldType string) bool {
-	return strings.HasPrefix(fieldType, "array[") && strings.HasSuffix(fieldType, "]")
+	return (strings.HasPrefix(fieldType, "array[") || strings.HasPrefix(fieldType, "nullablearray[")) && strings.HasSuffix(fieldType, "]")


Maybe if a field is optional but otherwise should reference an existing type? If we don't intend for it to be used that way let's just remove it; removing it should cause an error to be returned when the type can't be parsed. We can always add the functionality if we end up wanting it. My bad.

rmweir

LGTM

Add a new field attribute "pointer" to indicate that the generated client code for the field must be a pointer. This allows clients to differentiate between sending nil/leaving the value unset and sending an empty map or slice. This change also removes the `nullablestring` norman type introduced in 30f8d18 since schemas that need a pointer to a string can now use this field attribute. There are no libraries currently using this feature so it should be safe to remove. Example usage: ``` Labels map[string]string `json:"labels" norman:"pointer"` ``` Resulting API schema: ``` "labels": { "create": true, "nullable": true, "pointer": true, "type": "map[string]", "update": true } ``` Generated client code: ``` Labels *map[string]string `json:"labels,omitempty" yaml:"labels,omitempty"` ```

cmurphy · 2021-06-08T00:13:03Z

After talking with @ibuildthecloud it made more sense to make this an attribute of the field instead of its type. It is still used to generate a pointer type, as opposed to messing with the omitempty in the template.

Use in gke-operator: rancher/gke-operator#49
Rancher: rancher/rancher#32502

ibuildthecloud · 2021-06-08T18:38:13Z

The approach is good. LGTM on the design.

norman removed the 'nullablestring' type in favor of a 'pointer' attribute for field types[1]. In addition to string pointers, this can be used on maps and slices to indicate the generated client code should become a pointer, even if the original type was not. [1] rancher/norman#400

This was referenced May 19, 2021

Use norman:"pointer" for nullable fields rancher/gke-operator#49

Merged

Update norman and KEv2 operators rancher/rancher#32502

Merged

mjura previously approved these changes May 26, 2021

View reviewed changes

thedadams suggested changes May 26, 2021

View reviewed changes

rmweir reviewed May 29, 2021

View reviewed changes

cmurphy dismissed mjura’s stale review via b437ea0 June 2, 2021 16:52

cmurphy force-pushed the nullablecomplex branch from 4b8924a to b437ea0 Compare June 2, 2021 16:52

cmurphy requested review from rmweir and thedadams June 2, 2021 16:52

rmweir reviewed Jun 2, 2021

View reviewed changes

thedadams previously approved these changes Jun 2, 2021

View reviewed changes

cmurphy commented Jun 3, 2021

View reviewed changes

cmurphy dismissed thedadams’s stale review via 558a46c June 3, 2021 20:55

cmurphy force-pushed the nullablecomplex branch from b437ea0 to 558a46c Compare June 3, 2021 20:55

cmurphy requested review from rmweir and thedadams June 3, 2021 20:55

thedadams previously approved these changes Jun 3, 2021

View reviewed changes

rmweir suggested changes Jun 4, 2021

View reviewed changes

cmurphy dismissed thedadams’s stale review via d677f21 June 4, 2021 16:26

cmurphy force-pushed the nullablecomplex branch from 558a46c to d677f21 Compare June 4, 2021 16:26

thedadams previously approved these changes Jun 4, 2021

View reviewed changes

cmurphy requested a review from rmweir June 4, 2021 21:41

rmweir previously approved these changes Jun 6, 2021

View reviewed changes

cmurphy dismissed stale reviews from rmweir and thedadams via 6e4b2ba June 8, 2021 00:04

cmurphy force-pushed the nullablecomplex branch from d677f21 to 6e4b2ba Compare June 8, 2021 00:04

cmurphy changed the title ~~Add complex nullable types~~ Use norman:"pointer" for nullable fields Jun 8, 2021

ibuildthecloud merged commit 59b3523 into rancher:master Jun 8, 2021

cmurphy mentioned this pull request Jun 8, 2021

Set empty objects instead error on create rancher/gke-operator#44

Closed

cmurphy mentioned this pull request Jun 8, 2021

Change to 'pointer' for norman fields rancher/aks-operator#25

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use norman:"pointer" for nullable fields #400

Use norman:"pointer" for nullable fields #400

cmurphy commented May 19, 2021 •

edited

Loading

cmurphy commented May 19, 2021

cmurphy commented May 19, 2021

thedadams commented May 26, 2021

mjura left a comment

thedadams May 26, 2021

cmurphy Jun 2, 2021

rmweir left a comment

cmurphy commented Jun 1, 2021

rmweir left a comment

rmweir Jun 2, 2021

cmurphy Jun 3, 2021

rmweir Jun 4, 2021

cmurphy Jun 4, 2021

cmurphy left a comment

cmurphy Jun 3, 2021

rmweir Jun 4, 2021

rmweir left a comment

cmurphy commented Jun 8, 2021 •

edited

Loading

ibuildthecloud commented Jun 8, 2021

	return "*" + getTypeString(false, typeName[len("nullable"):len(typeName)], schema, schemas)
	return "*" + getTypeString(false, strings.TrimPrefix(typeName, "nullable"), schema, schemas)

	case "boolean":
	name = "bool"
	case "float":
	name = "float64"
	case "int":
	name = "int64"

Use norman:"pointer" for nullable fields #400

Use norman:"pointer" for nullable fields #400

Conversation

cmurphy commented May 19, 2021 • edited Loading

cmurphy commented May 19, 2021

cmurphy commented May 19, 2021

thedadams commented May 26, 2021

mjura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmweir left a comment

Choose a reason for hiding this comment

cmurphy commented Jun 1, 2021

rmweir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmurphy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmweir left a comment

Choose a reason for hiding this comment

cmurphy commented Jun 8, 2021 • edited Loading

ibuildthecloud commented Jun 8, 2021

cmurphy commented May 19, 2021 •

edited

Loading

cmurphy commented Jun 8, 2021 •

edited

Loading