Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect duplicate name error with extensions #161

Closed
ghost opened this issue Jul 13, 2014 · 9 comments
Closed

Incorrect duplicate name error with extensions #161

ghost opened this issue Jul 13, 2014 · 9 comments

Comments

@ghost
Copy link

ghost commented Jul 13, 2014

Here's a test case:

message Base {
  extensions 100 to max;
}

message Derived1 {
  extend Base {
    optional Derived1 derived = 101;
  }

  optional string foo = 1;
}

message Derived2 {
  extend Base {
    optional Derived2 derived = 102;
  }

  optional int32 foo = 1;
}

Derived1.derived and Derived2.derived should not clash, as they're in different messages. ProtoBuf.js incorrectly believes derived should be a field of Base and thinks I've declared it twice:

Error: Duplicate name in namespace Message .Base: derived
@dcodeIO
Copy link
Member

dcodeIO commented Jul 13, 2014

Mh, does this work with the official protoc?

If so, what is the expected message structure after building? Is it?

message Derived1 {
    message Base {
        extensions 100 to max;
        optional Derived1 derived = 101;
    }
    optional string foo = 1;
}
...

Edit: Looked through the documentation. At the moment, nested extensions are processed just as if they were declared on the top level. There is no mechanism like SetExtension in place. I assume we'll need that now.

@SirZach
Copy link

SirZach commented Aug 20, 2014

Any word on this? I just started a project and imported 100+ .proto files that all reuse the same name with extensions that protobuf is currently bombing on.

If someone could point me in the right direction, I could take a crack at it myself.

@dcodeIO
Copy link
Member

dcodeIO commented Aug 20, 2014

At the moment, there is no facility like setExtension like in the official library. I've started to work on this but it's still a quite long road, requiring breaking API changes eventually.

@SirZach
Copy link

SirZach commented Aug 21, 2014

I admit to being very new at protobufs so I'm not sure how to continue your work. Do you have any sort of API in mind? Maybe I could start with submitting some unit tests?

@dcodeIO
Copy link
Member

dcodeIO commented Aug 22, 2014

Got an idea. Let's say we have a proto like this:

message A {
    required string id = 1;
}

extend A {
    optional string id = 2; // fqn: .id
}

message B {
    extend A {
        optional string id = 3; // fqn: .B.id
    }
    ...
}

Considering nesting like this, the API could look like:

var A = builder.build("A");
var a = new A();
a.set("id", 123);
a.set(".id", 234); // using the fully qualified name of the extended field as the key
a.set(".B.id", 345); // using the fqn as well

What do you think?

@SirZach
Copy link

SirZach commented Aug 22, 2014

Given your proto schema...do you envision be able to do something like...

var A = builder.build('A');
var B = builder.build('B');
a.set({ // optionally allow for set using an object literal
  'id': 123,
  '.id': 234
});
b.set('id', 345);
a.set('.B', b);

@dcodeIO
Copy link
Member

dcodeIO commented Aug 22, 2014

You can use an object to construct a message already. This internally uses Message#set, so with an extension mechanism like the proposed, it would be:

var A = builder.build("A");
var a = new A({ 'id': 123, '.id': 234 });

@dcodeIO
Copy link
Member

dcodeIO commented Aug 23, 2014

I've implemented this with 3.5.0. Extended fields now use their fully qualified name as the field's name. There are no setters for extended fields, so the recommended way of setting them is:

message A {
    required string id = 1;
}

extend A {
    optional string id = 2; // fqn: .id
}

message B {
    extend A {
        optional string id = 3; // fqn: .B.id
    }
    ...
}
var A = builder.build("A");
var a = new A();
a.set(".id", 234); // or a['.id'] = 234;
a.set(".B.id", 345); // or a['.B.id'] = 345;
...

Please let me know if it is working for you.

@ghost
Copy link
Author

ghost commented Aug 24, 2014

Works great; thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants