[
https://issues.apache.org/jira/browse/THRIFT-2232?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13803320#comment-13803320
]
Jens Geyer edited comment on THRIFT-2232 at 10/23/13 9:02 PM:
--------------------------------------------------------------
Hi Ben,
{quote}
The various generated IsSetXYZ() methods just check for magic values of their
respective fields.
This is obviously broken, and also diverges from the implementation in other
languages.
{quote}
I just ran the test with
{code}
struct Test {
1: optional string one
2: required string two
3: string three
4: optional i32 four
5: required i32 five
6: i32 six
}
{code}
which gave me this
{code}
func (p *Test) IsSetOne() bool {
return p.One != ""
}
func (p *Test) IsSetFour() bool {
return p.Four != 0
}
{code}
Whether or not this is sufficient may be questionable (especially IsSetFour()
seems a bit heuristic), but I can't find any reference to "reflect" in the Go
generator or in IsSetXxx() at all. What am I doing wrong?
EDIT: After reading it again, you probably mean exactly that: the comparison
against empty strings and 0 integers. The "magic" did send me on the wrong
path, sorry.
{quote}
Here are some things I'd like to maintain:
- do not force users to manually maintain a bit vector of IsSet information
{quote}
Some languages use setter methods, where the value setter also switches the
isset flag to true. Maintaining a bit vector would be an implementation detail,
so I agree on that.
{quote}
The sad thing is that I can't imagine a solution that accomplishes all of the
above. Since these are still relatively early days (...), I am inclined to give
up the backwards compatibility goal.
{quote}
The entire Go code has been rewritten not so far ago to keep up with the Go 1.1
release. From that viewpoint, backwards compatibility is relative, but breaking
compatibility twice within only a few months sounds not like such a great plan
to me. On the other hand, if the implementation participates from it, a few
breaking changes that are easy to fix may be an acceptable price. It always
depends on the changes, IMHO.
{quote}
interfaces
{quote}
One point where I still think interfaces could be a solution is the (still
unsolved) problem with complex map keys, THRIFT-2063.
{quote}
My proposal is to do something akin to what protobufs does to solve this
problem. (It's also similar to what other popular packages do for things like
this... e.g., the way that the `gorp` ORM package deals with nullable database
columns) In particular, we would represent an optional field as a pointer, and
use the `nil` value to represent absence. Required fields could either (a) also
be represented as pointers and make their containing structs invalid when
absent, or (b) be represented as non-pointer fields like the current thrift Go
impl does for all fields.
{quote}
I agree on that. We use similar approaches at other places.
{quote}
list<>s etc would hopefully be unaffected by this proposal... they can still be
empty, and that's all we need as far as presence/absence info (correct me if
I'm mistaken!).
{quote}
Is an empty list really the same as a non-existent list? In my opinion
containers should be treaten the same way as structs, at least regarding that
point.
{quote}
The above begs the question of whether there can/should be a central lib of Go
thrift helpers that people can add to their projects (rather than simply
embedding all of these, redundantly, in every generated thrift Go package).
{quote}
If you mean that the central helpers library would be something outside of the
Thrift source tree, I don't agree. We strive to minimize external dependencies
as much as it makes sense in order to reduce problems on the different
platforms supported by Thrift, so this would be clearly counter-productive. If
we need these helpers, they should be integrated into the Go Thrift runtime
package (aka. library).
was (Author: jensg):
Hi Ben,
{quote}
The various generated IsSetXYZ() methods just check for magic values of their
respective fields.
This is obviously broken, and also diverges from the implementation in other
languages.
{quote}
I just ran the test with
{code}
struct Test {
1: optional string one
2: required string two
3: string three
4: optional i32 four
5: required i32 five
6: i32 six
}
{code}
which gave me this
{code}
func (p *Test) IsSetOne() bool {
return p.One != ""
}
func (p *Test) IsSetFour() bool {
return p.Four != 0
}
{code}
Whether or not this is sufficient may be questionable (especially IsSetFour()
seems a bit heuristic), but I can't find any reference to "reflect" in the Go
generator or in IsSetXxx() at all. What am I doing wrong?
{quote}
Here are some things I'd like to maintain:
- do not force users to manually maintain a bit vector of IsSet information
{quote}
Some languages use setter methods, where the value setter also switches the
isset flag to true. Maintaining a bit vector would be an implementation detail,
so I agree on that.
{quote}
The sad thing is that I can't imagine a solution that accomplishes all of the
above. Since these are still relatively early days (...), I am inclined to give
up the backwards compatibility goal.
{quote}
The entire Go code has been rewritten not so far ago to keep up with the Go 1.1
release. From that viewpoint, backwards compatibility is relative, but breaking
compatibility twice within only a few months sounds not like such a great plan
to me. On the other hand, if the implementation participates from it, a few
breaking changes that are easy to fix may be an acceptable price. It always
depends on the changes, IMHO.
{quote}
interfaces
{quote}
One point where I still think interfaces could be a solution is the (still
unsolved) problem with complex map keys, THRIFT-2063.
{quote}
My proposal is to do something akin to what protobufs does to solve this
problem. (It's also similar to what other popular packages do for things like
this... e.g., the way that the `gorp` ORM package deals with nullable database
columns) In particular, we would represent an optional field as a pointer, and
use the `nil` value to represent absence. Required fields could either (a) also
be represented as pointers and make their containing structs invalid when
absent, or (b) be represented as non-pointer fields like the current thrift Go
impl does for all fields.
{quote}
I agree on that. We use similar approaches at other places.
{quote}
list<>s etc would hopefully be unaffected by this proposal... they can still be
empty, and that's all we need as far as presence/absence info (correct me if
I'm mistaken!).
{quote}
Is an empty list really the same as a non-existent list? In my opinion
containers should be treaten the same way as structs, at least regarding that
point.
{quote}
The above begs the question of whether there can/should be a central lib of Go
thrift helpers that people can add to their projects (rather than simply
embedding all of these, redundantly, in every generated thrift Go package).
{quote}
If you mean that the central helpers library would be something outside of the
Thrift source tree, I don't agree. We strive to minimize external dependencies
as much as it makes sense in order to reduce problems on the different
platforms supported by Thrift, so this would be clearly counter-productive. If
we need these helpers, they should be integrated into the Go Thrift runtime
package (aka. library).
> IsSet* broken in Go
> -------------------
>
> Key: THRIFT-2232
> URL: https://issues.apache.org/jira/browse/THRIFT-2232
> Project: Thrift
> Issue Type: Bug
> Components: Go - Compiler
> Reporter: Ben Sigelman
> Labels: isset
> Fix For: 0.9.2
>
>
> The various generated IsSetXYZ() methods just check for magic values of their
> respective fields. This is obviously broken, and also diverges from the
> implementation in other languages.
> I am willing and able to fix this myself, but I don't want to start on any
> impl until we can decide on an approach.
> At this point, though, optional fields in Go are basically useless if one's
> application makes use of the magic "absence" value.
--
This message was sent by Atlassian JIRA
(v6.1#6144)
