Thriftpy / thriftpy2

Pure python approach of Apache Thrift.
MIT License
572 stars 91 forks source link

Duplicate structs in a namespace should fail to parse #166

Closed aawilson closed 3 years ago

aawilson commented 3 years ago

This isn't strictly specified in the IDL, but recent versions of Apache thrift will fail to compile bindings when a namespace contains duplicate structs. With the following foo.thrift

With the following foo.thrift

struct Foo {
    1:i64 foo_id
}

struct Foo {
    1:string foo_idstring
}
thrift --gen rb -r -o . -I ./ ./foo.thrift
[ERROR:./foo.thrift:8] (last token was '')
Type "Foo" is already defined.
[WARNING:./foo.thrift:8] Duplicate typename Foo found in foo

I don't know exactly when it started because I'm having trouble compiling some of the earlier thrift versions, but it's at least since 0.10.0. The thrift source actually includes a test suite that might be interesting to go over with a fine-toothed comb to see if there are error cases parser.py doesn't catch.

I'm not super-familiar with lex/yacc to know if there is a better way, but it feels like the easy way to do this is to add a check to this function in parser.py:


def _add_thrift_meta(key, val):
    thrift = thrift_stack[-1]

    if not hasattr(thrift, '__thrift_meta__'):
        meta = collections.defaultdict(list)
        setattr(thrift, '__thrift_meta__', meta)
    else:
        meta = getattr(thrift, '__thrift_meta__')

    meta[key].append(val)

I can throw up a quick PoC PR for this, it's little enough work that if there's some better way of doing it I won't feel bad for spending the time.

aawilson commented 3 years ago

Closing since #167 merged.