[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15876766#comment-15876766
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user asfgit closed the pull request at:

https://github.com/apache/thrift/pull/1156


> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15875530#comment-15875530
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user Jens-G commented on the issue:

https://github.com/apache/thrift/pull/1156
  
Ok, thanks.


> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15874483#comment-15874483
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1156
  
@Jens-G All tests green and rebased from master.


> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15873147#comment-15873147
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1156
  
Whoops, I must have missed that one. Should be fixed now.


> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15873145#comment-15873145
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user Jens-G commented on the issue:

https://github.com/apache/thrift/pull/1156
  
```
src/common/clientserver_test.go:61: cannot use handler (type 
*MockThriftTest) as type thrifttest.ThriftTest in argument to GetServerParams:
*MockThriftTest does not implement thrifttest.ThriftTest (wrong type 
for TestSet method)
have TestSet(map[int32]struct {}) (map[int32]struct {}, error)
want TestSet([]int32) ([]int32, error)
src/common/clientserver_test.go:226: cannot use s (type map[int32]struct 
{}) as type []int32 in argument to client.TestSet
FAILcommon [build failed]
make[2]: *** [check] Error 2
make[2]: Leaving directory `/thrift/src/test/go'
make[1]: *** [check-recursive] Error 1
make[1]: Leaving directory `/thrift/src/test'
make: *** [check-recursive] Error 1

```


> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15863449#comment-15863449
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1156
  
@Jens-G Squashed commits and rebased from master.


> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15858121#comment-15858121
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user dcelasun commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1156#discussion_r100089847
  
--- Diff: lib/go/test/tests/thrifttest_driver.go ---
@@ -162,7 +162,7 @@ func (p *ThriftTestDriver) Start() {
t.Fatal("TestStringMap failed")
}
 
-   setTestInput := map[int32]struct{}{1: {}, 2: {}, 3: {}}
+   setTestInput := []int32{1, 2, 3}
--- End diff --

Yeah, it's also finally serializable to JSON for non-primitive types.


> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15858111#comment-15858111
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user jeking3 commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1156#discussion_r100088653
  
--- Diff: lib/go/test/tests/thrifttest_driver.go ---
@@ -162,7 +162,7 @@ func (p *ThriftTestDriver) Start() {
t.Fatal("TestStringMap failed")
}
 
-   setTestInput := map[int32]struct{}{1: {}, 2: {}, 3: {}}
+   setTestInput := []int32{1, 2, 3}
--- End diff --

Syntax looks much better than before.


> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15857894#comment-15857894
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1156
  
I finally hade some time to take a look at this. Duplicate detection is now 
implemented with `reflect.DeepEqual`. It isn't really ideal, but Go lacks a way 
of defining equality for arbitrary types, so this is the only possible 
implementation that supports `set` for any `T`.


> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[James E. King, III (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15854483#comment-15854483
 ] 

James E. King, III commented on THRIFT-4011:


This was described as a breaking change.  I am doing something similar for 
perl, switching the type for a set to Set::Scalar.  I want to remain backwards 
compatible as an option while having the new behavior the default, and I was 
going to add a compiler option like {{--gen perl:hash_sets}} to make it use 
hashes like it always has.  Do we need a go compiler option here to emit 
backwards-compatible or new code here depending?

> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15838409#comment-15838409
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user jeking3 commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1156#discussion_r97858435
  
--- Diff: lib/go/test/tests/thrifttest_handler.go ---
@@ -96,7 +96,7 @@ func (p *ThriftTestHandler) TestStringMap(thing 
map[string]string) (r map[string
return thing, nil
 }
 
-func (p *ThriftTestHandler) TestSet(thing map[int32]struct{}) (r 
map[int32]struct{}, err error) {
+func (p *ThriftTestHandler) TestSet(thing []int32) (r []int32, err error) {
--- End diff --

As long as it is a compile time failure if someone doesn't change it, 
that's good.  In C++ (C++03, or C++11 without the "override" keyword being 
used) if you change the signature of a virtual method, anything that overrides 
it will silently define a new method instead of overriding what you want.  It's 
pretty messy.


> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15838285#comment-15838285
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user dcelasun commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1156#discussion_r97843055
  
--- Diff: lib/go/test/tests/thrifttest_handler.go ---
@@ -96,7 +96,7 @@ func (p *ThriftTestHandler) TestStringMap(thing 
map[string]string) (r map[string
return thing, nil
 }
 
-func (p *ThriftTestHandler) TestSet(thing map[int32]struct{}) (r 
map[int32]struct{}, err error) {
+func (p *ThriftTestHandler) TestSet(thing []int32) (r []int32, err error) {
--- End diff --

Yes, this is a breaking change which was originally discussed in 
THRIFT-4011; I only started working on this PR after I was given the go ahead.

> How will this be communicated?

I would imagine posts to the mailing lists and an announcement on the 
website, several weeks in advance of a new release?

> What happens to existing handlers that implement the older method?

Compiling IDLs with this patch will change the signature of any RPC or 
struct in the generated Go code, so it's very easy to catch at compile time and 
make the changes.


> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15838271#comment-15838271
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user jeking3 commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1156#discussion_r97841167
  
--- Diff: lib/go/test/tests/thrifttest_handler.go ---
@@ -96,7 +96,7 @@ func (p *ThriftTestHandler) TestStringMap(thing 
map[string]string) (r map[string
return thing, nil
 }
 
-func (p *ThriftTestHandler) TestSet(thing map[int32]struct{}) (r 
map[int32]struct{}, err error) {
+func (p *ThriftTestHandler) TestSet(thing []int32) (r []int32, err error) {
--- End diff --

This looks like a breaking change; any generated go implementation's 
handler will need to change.  How will this be communicated?  What happens to 
existing handlers that implement the older method?


> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15823690#comment-15823690
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user Jens-G commented on the issue:

https://github.com/apache/thrift/pull/1156
  
+1

Sent from mobile device, please ignore spelling mistakes.

Von: Duru Can Celasun
Gesendet: 16.01.2017 08:00
An: apache/thrift
Cc: Jens Geyer; Mention
Betreff: Re: [apache/thrift] THRIFT-4011 Use slices for Thrift sets (#1156)


Good intentions, absolutely, but the implementation ..

FWIW, I agree. However, we still need a solution and as I explained above, 
we don't have a way of returning an error here. That leaves us with silent 
deduplication (during serialization) in lib/go/thrift. Are you OK with that?

-
You are receiving this because you were mentioned.
Reply to this email directly, view it on 
GitHub, or 
mute the 
thread.



> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15823547#comment-15823547
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1156
  
> Good intentions, absolutely, but the implementation ..

FWIW, I agree. However, we still need a solution and as I explained above, 
we don't have a way of returning an error here. That leaves us with silent 
deduplication (during serialization) in lib/go/thrift. Are you OK with that?


> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15823384#comment-15823384
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user Jens-G commented on the issue:

https://github.com/apache/thrift/pull/1156
  
Oopsie. After I read the word "map" above I must have been mentally 
switched to maps somehow.  My mistake.

Nevertheless what I said about panic and errors in general above still 
holds true. At this time we have exactly 0 (zero) panics in the code under 
/lib/go/thrift. So I would argue that introducing a panic at least breaks the 
usual patterns of that library.

>  Speaking of which, panicking in case of programmer error is very common 
and idiomatic in Go. The standard library is full of such panics 

Put four Go experts together and you get five opinions, all idiomatic. 
Especially about the high and lofty Go error handling design principles:

> In Go, error handling is important. **The language's design and 
conventions encourage you to explicitly check for errors** where they occur (as 
distinct from the convention in other languages of throwing exceptions and 
sometimes catching them). 

Or mostly assigning them to `_` as many people do in Go. That's for the 
"encourage you to explicitly check for errors" part. In contrast, an exception 
must be caught, not only "sometimes", because there is no other error handling 
option. 

> The decision to not include exceptions in Go is an example of its 
**simplicity** and orthogonality. Using multiple return values and a simple 
convention, **Go solves the problem** of letting programmers know when things 
have gone wrong and **reserves panic for the truly exceptional**.

I can't see any "simplicity" in replacing exceptions by panics, 
"encouraging" error handling by leading people into misusing the `_` operator 
and at the same time claiming "*this time, we get it right*". Go's error 
handling is FUBAR from the beginning to the end. Good intentions, absolutely, 
but the implementation  All personal opinion, of course, 




> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15823163#comment-15823163
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user dcelasun commented on the issue:

https://github.com/apache/thrift/pull/1156
  
Hey @Jens-G, I'm not sure I follow you, what does this have anything to do 
with maps? Assuming you meant sets, the docs say:

> An unordered set of unique elements. Translates to an STL set, Java 
HashSet, set in Python, etc. Note: PHP does not support sets, so it is treated 
similar to a List

Similar to PHP, Go does not have a native type for sets, so the best thing 
to do is to treat it similar to a list.

> Given that, I would say it could be one option to error, when the user 
inserts a duplicate.

This is not possible, because the caller doesn't "insert" anything, they 
simply return a slice. Consider the following:

```thrift
service Foo {
set bar() throws (1: Something error)
}
```

This generates an interface called `Foo` with the following method:

```go
type Foo interface {
Bar() ([]string, error)
}
```

So the user simply returns a string slice, the Thrift library has no 
control over it. Once `Foo` returns, it's too late for Thrift itself to return 
an error, only panic. Speaking of which, panicking in case of ***programmer 
error*** is very common and idiomatic in Go. The standard library is full of 
such panics (e.g search for "misuse" 
[here](https://golang.org/src/sync/waitgroup.go)). I would consider returning a 
non-unique slice for a Thrift set a programming error (and hence deserving a 
panic), but if you disagree, I can update the PR with deduplication in the 
library.


> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15823148#comment-15823148
 ] 

ASF GitHub Bot commented on THRIFT-4011:


Github user Jens-G commented on the issue:

https://github.com/apache/thrift/pull/1156
  
http://thrift.apache.org/docs/types states that (as one would expect) a 
Thrift map is defined as "A map of **strictly unique keys** to values. 
Translates to an STL map, Java HashMap, PHP associative array, Python/Ruby 
dictionary, etc.". 

Given that, I would say it could be one option to error, when the user 
inserts a duplicate. On the other hand, in that case the better option could be 
to simply replace the current value. 

But **what should not happen is that serialized data come in with 
duplicated keys**. That would be clearly an error as it is a violation of the 
rule above. In that case it might be absolutely ok to return some kind of error.

Re `panic()`, from my understanding of Go that's not the idiomatic way to 
go, since everything form tha above is an easily recoverable error.

Further reading:
 * https://blog.golang.org/error-handling-and-go
 * https://golang.org/doc/effective_go.html#panic
 * 
http://stackoverflow.com/questions/28472922/when-to-use-os-exit-and-panic-in-golang



> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (THRIFT-4011) Sets of Thrift structs generate Go code that can't be serialized to JSON

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/THRIFT-4011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15823094#comment-15823094
 ] 

ASF GitHub Bot commented on THRIFT-4011:


GitHub user dcelasun opened a pull request:

https://github.com/apache/thrift/pull/1156

THRIFT-4011 Use slices for Thrift sets

As discussed in 
[THRIFT-4011](https://issues.apache.org/jira/browse/THRIFT-4011), this commit 
changes the Go generator to use slices, instead of maps for Thrift sets.

I've specifically didn't touch the Go library since there was no agreement 
on panicking for duplicates. We have three options:

1. Leave it as is and add documentation stating deduplication is the 
caller's responsibility.
2. Silently deduplicate before serialization.
3. panic on duplicates.

2 and 3 probably requires 
[`reflect.DeepEqual`](https://golang.org/pkg/reflect/#DeepEqual), which is not 
ideal.

@Jens-G Thoughts?

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dcelasun/thrift master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/thrift/pull/1156.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1156


commit 0e2e8c0b041300dafff641e19848a1e46df32bc6
Author: D. Can Celasun 
Date:   2017-01-15T09:53:19Z

THRIFT-4011 Use slices for Thrift sets

This commit changes the Go generator to use slices, instead of maps for 
Thrift sets.




> Sets of Thrift structs generate Go code that can't be serialized to JSON
> 
>
> Key: THRIFT-4011
> URL: https://issues.apache.org/jira/browse/THRIFT-4011
> Project: Thrift
>  Issue Type: Bug
>  Components: Go - Compiler
>Reporter: Can Celasun
>
> Consider the following structs:
> {code}
> struct Foo {
>   1: optional string foo
> }
> struct Bar {
>   1: optional set foos
> }
> {code}
> This compiles into the following Go code:
> {code}
> type Bar struct {
>   Foos map[*Foo]struct{} `thrift:"foos,1" db:"foos" json:"foos,omitempty"`
> }
> {code}
> Even though the generated code has tags for JSON support, Bar can't be 
> serialized to JSON:
> {code}
> json: unsupported type: map[*Foo]struct {}
> {code}
> One solution would be to use slices, not maps, for Thrift sets. Thoughts?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)