You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by bgould <gi...@git.apache.org> on 2016/11/01 04:17:09 UTC
[GitHub] thrift pull request #1120: THRIFT-3301: Naming collisions possible in genera...
GitHub user bgould opened a pull request:
https://github.com/apache/thrift/pull/1120
THRIFT-3301: Naming collisions possible in generated Java code
Switched to fully-qualified names for all class name references in generated code. Added test IDL to verify generated code compiles successfully.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bgould/thrift THRIFT-3301
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1120.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 #1120
----
commit c6c5a054955c1b76fa498bb74388b5ddc8f36c14
Author: BCG <bg...@users.noreply.github.com>
Date: 2016-11-01T03:32:18Z
Switched to fully-qualified names for all class name references in generated code
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1120: THRIFT-3301: Naming collisions possible in genera...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1120
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1120: THRIFT-3301: Naming collisions possible in generated Jav...
Posted by bgould <gi...@git.apache.org>.
Github user bgould commented on the issue:
https://github.com/apache/thrift/pull/1120
Also one thing to note is that the Java code generated from following valid IDL will still fail to compile because the names match Java primitive types... I know of no way to avoid this problem:
```thrift
struct int {
1: i32 val
}
struct boolean {
1: bool val
}
struct long {
1: i64 val
}
struct short {
1: i16 short
}
struct char {
1: i16 val
}
struct Primitives {
1: i32 int,
2: i64 long,
3: i16 short,
4: bool boolean,
5: i16 char
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1120: THRIFT-3301: Naming collisions possible in generated Jav...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1120
Does the cmake install job need to be updated as well?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1120: THRIFT-3301: Naming collisions possible in generated Jav...
Posted by bgould <gi...@git.apache.org>.
Github user bgould commented on the issue:
https://github.com/apache/thrift/pull/1120
@jeking3
Yes exactly. I added it to the build process that compiles the java unit tests... so if some change to compiler introduces a regression, the IDL in that file should fail to compile. I didn't add a unit test for this as I felt that the compilation itself was the test.
Consider the following IDL:
struct String {
1: string val
}
struct Object {
1: map<string, String> somemap
}
It would be possible to make a modification to the compiler that erroneously generates the map type as `java.util.Map<String, String>` instead of `java.util.Map<java.lang.String, String>` or to add some reference in the generated code to `java.lang.Object` that is not fully-qualified... however with the generated classes above in the `thrift.test` package, `javac` should flag the unqualified reference(s) as ambiguous.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1120: THRIFT-3301: Naming collisions possible in genera...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1120#discussion_r87700206
--- Diff: test/JavaTypes.thrift ---
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+namespace java thrift.test
+
+struct Integer {
+ 1: i32 val
+}
+
+struct String {
+ 1: string val
+}
+
+struct Boolean {
+ 1: bool val
+}
+
+struct Double {
+ 1: double val
+}
+
+struct Long {
+ 1: i64 val
+}
+
+struct Byte {
+ 1: byte val
+}
+
+struct Float {
+ 1: double val
+}
+
+struct List {
+ 1: list<string> vals
+}
+
+struct ArrayList {
+ 1: list<string> vals
+}
+
+struct SortedMap {
+ 1: map<string, string> vals
+}
+
+struct TreeMap {
+ 1: map<string, string> vals
+}
+
+struct HashMap {
+ 1: map<string, String> vals
+}
+
+struct Map {
+ 1: map<double, Double> vals
+}
+
+struct Object {
+ 1: Integer integer,
+ 2: String str,
+ 3: Boolean boolean_field,
+ 4: Double dbl,
+ 5: Byte bite,
+ 6: map<i32, Integer> intmap,
+ 7: Map somemap,
+}
+
--- End diff --
Do we need the same for exception?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1120: THRIFT-3301: Naming collisions possible in genera...
Posted by bgould <gi...@git.apache.org>.
Github user bgould commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1120#discussion_r90040746
--- Diff: test/JavaTypes.thrift ---
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+namespace java thrift.test
+
+struct Integer {
+ 1: i32 val
+}
+
+struct String {
+ 1: string val
+}
+
+struct Boolean {
+ 1: bool val
+}
+
+struct Double {
+ 1: double val
+}
+
+struct Long {
+ 1: i64 val
+}
+
+struct Byte {
+ 1: byte val
+}
+
+struct Float {
+ 1: double val
+}
+
+struct List {
+ 1: list<string> vals
+}
+
+struct ArrayList {
+ 1: list<string> vals
+}
+
+struct SortedMap {
+ 1: map<string, string> vals
+}
+
+struct TreeMap {
+ 1: map<string, string> vals
+}
+
+struct HashMap {
+ 1: map<string, String> vals
+}
+
+struct Map {
+ 1: map<double, Double> vals
+}
+
+struct Object {
+ 1: Integer integer,
+ 2: String str,
+ 3: Boolean boolean_field,
+ 4: Double dbl,
+ 5: Byte bite,
+ 6: map<i32, Integer> intmap,
+ 7: Map somemap,
+}
+
--- End diff --
that is probably a good idea, will get that added.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1120: THRIFT-3301: Naming collisions possible in generated Jav...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1120
> The following tests FAILED:
> 40 - JavaTest (Failed)
> Errors while running CTest
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1120: THRIFT-3301: Naming collisions possible in generated Jav...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1120
I'm not seeing why JavaTypes was introduced - was it simply used to manually check the build for output of every native type? Is the result from JavaTypes compilation used in some unit test that would fail if it didn't produce the correct output? If not, does it belong in the distribution?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1120: THRIFT-3301: Naming collisions possible in generated Jav...
Posted by bgould <gi...@git.apache.org>.
Github user bgould commented on the issue:
https://github.com/apache/thrift/pull/1120
Thanks @nsuke I'm pretty sure that was the problem. I created a new IDL file for validating this change, and I've now added it to tests/Makefile.am
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1120: THRIFT-3301: Naming collisions possible in generated Jav...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the issue:
https://github.com/apache/thrift/pull/1120
@bgould the failing CI job is make dist tarball verification.
It's typically caused by missing `EXTRA_DIST` entries for newly added files in `Makefile.am`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1120: THRIFT-3301: Naming collisions possible in genera...
Posted by bgould <gi...@git.apache.org>.
Github user bgould commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1120#discussion_r90040582
--- Diff: lib/java/test/org/apache/thrift/test/TestClient.java ---
@@ -576,7 +581,7 @@ public static void main(String [] args) {
goodbye.string_thing = "Goodbye4";
goodbye.byte_thing = (byte)4;
goodbye.i32_thing = 4;
- goodbye.i64_thing = (long)4;
+ goodbye.i64_thing = 4;
--- End diff --
Yes that is unrelated... I think my IDE settings are such that it was removed automatically because it was an unncessary... no problem for me to restore it if you prefer.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1120: THRIFT-3301: Naming collisions possible in genera...
Posted by bgould <gi...@git.apache.org>.
Github user bgould commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1120#discussion_r90043799
--- Diff: test/JavaTypes.thrift ---
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+namespace java thrift.test
+
+struct Integer {
+ 1: i32 val
+}
+
+struct String {
+ 1: string val
+}
+
+struct Boolean {
+ 1: bool val
+}
+
+struct Double {
+ 1: double val
+}
+
+struct Long {
+ 1: i64 val
+}
+
+struct Byte {
+ 1: byte val
+}
+
+struct Float {
+ 1: double val
+}
+
+struct List {
+ 1: list<string> vals
+}
+
+struct ArrayList {
+ 1: list<string> vals
+}
+
+struct SortedMap {
+ 1: map<string, string> vals
+}
+
+struct TreeMap {
+ 1: map<string, string> vals
+}
+
+struct HashMap {
+ 1: map<string, String> vals
+}
+
+struct Map {
+ 1: map<double, Double> vals
+}
+
+struct Object {
+ 1: Integer integer,
+ 2: String str,
+ 3: Boolean boolean_field,
+ 4: Double dbl,
+ 5: Byte bite,
+ 6: map<i32, Integer> intmap,
+ 7: Map somemap,
+}
+
--- End diff --
As it happens, adding the Exception type did expose a naming collision... I will track it down and push an update when fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1120: THRIFT-3301: Naming collisions possible in generated Jav...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1120
I think a number of those things are in the "reserved" keyword list for the thrift compiler, so that's okay. Nice, I'll mark this one approved.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1120: THRIFT-3301: Naming collisions possible in generated Jav...
Posted by bgould <gi...@git.apache.org>.
Github user bgould commented on the issue:
https://github.com/apache/thrift/pull/1120
@jeking3 @Jens-G
Looks my last commit caused travis-ci to fail... from what I can tell it doesn't look like it was related to my changes tho (it in the test suite during Haskell generation it looks like)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---