You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by "Will-Lo (via GitHub)" <gi...@apache.org> on 2023/04/27 03:08:57 UTC
[GitHub] [gobblin] Will-Lo opened a new pull request, #3688: [GOBBLIN-1826] Change isAssignableFrom() to isSuperTypeOf() per Guava 20 javadocs to…
Will-Lo opened a new pull request, #3688:
URL: https://github.com/apache/gobblin/pull/3688
… fix bug in Hive registration where classes can escape type casts
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
### JIRA
- [ ] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-1826
### Description
- [ ] Here are some details about my PR, including screenshots (if applicable):
After the Guava 20 upgrade, there is a bug in the Hive registration code.
When setting table properties, `HiveTable` calls this function in `HiveRegistrationUnit` to set Properties
```
protected static <T> Optional<T> populateField(State state, String key, TypeToken<T> token) {
```
Since it returns a type generic Optional, it is possible to assign an incorrect class to a field. So an Optional<Long> can actually be holding a String value and causes a Class cast exception.
What was happening is that in Guava 20, `isAssignableFrom` is deprecated from the `TypeToken` class. Instead, the following code pattern was used:
```
} else if (new TypeToken<Long>() {}.getRawType().isAssignableFrom(token.getClass())) {
```
However, in this case getRawType() would have returned java.Lang.Long, so it would not be assignable to the token class but rather the base class.
To avoid all this headache, and to also properly validate the List<String> token type, we should be using `.isSupertypeOf()` in lieu of `isAssignableFrom()`.
This time also wrote a unit test to validate the runtime behavior of each type.
### Tests
- [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
Unit tests
### Commits
- [ ] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] Will-Lo merged pull request #3688: [GOBBLIN-1826] Change isAssignableFrom() to isSuperTypeOf() per Guava 20 javadocs to…
Posted by "Will-Lo (via GitHub)" <gi...@apache.org>.
Will-Lo merged PR #3688:
URL: https://github.com/apache/gobblin/pull/3688
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3688: [GOBBLIN-1826] Change isAssignableFrom() to isSuperTypeOf() per Guava 20 javadocs to…
Posted by "Will-Lo (via GitHub)" <gi...@apache.org>.
Will-Lo commented on code in PR #3688:
URL: https://github.com/apache/gobblin/pull/3688#discussion_r1179270277
##########
gobblin-hive-registration/src/test/java/org/apache/gobblin/hive/HiveTableTest.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.
+ */
+
+package org.apache.gobblin.hive;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.testng.annotations.Test;
+
+import junit.framework.Assert;
+
+import org.apache.gobblin.configuration.State;
+
+public class HiveTableTest {
Review Comment:
Yeah I originally had that too, the issue was that the HiveRegistrationUnit Builder is abstract and only implemented in the HiveTable class. I could create a mock or test class but would rather have a test that goes through more of the production code to make sure less things are missed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3688: [GOBBLIN-1826] Change isAssignableFrom() to isSuperTypeOf() per Guava 20 javadocs to…
Posted by "Will-Lo (via GitHub)" <gi...@apache.org>.
Will-Lo commented on code in PR #3688:
URL: https://github.com/apache/gobblin/pull/3688#discussion_r1179268362
##########
gobblin-hive-registration/src/main/java/org/apache/gobblin/hive/HiveRegistrationUnit.java:
##########
@@ -125,14 +125,13 @@ protected void populateSerDeFields(State state) {
protected static <T> Optional<T> populateField(State state, String key, TypeToken<T> token) {
if (state.contains(key)) {
Optional<T> fieldValue;
-
- if (new TypeToken<Boolean>() {}.getRawType().isAssignableFrom(token.getClass())) {
+ if (new TypeToken<Boolean>(){}.isSupertypeOf(token)) {
fieldValue = (Optional<T>) Optional.of(state.getPropAsBoolean(key));
- } else if (new TypeToken<Integer>() {}.getRawType().isAssignableFrom(token.getClass())) {
+ } else if (new TypeToken<Integer>(){}.isSupertypeOf(token)) {
fieldValue = (Optional<T>) Optional.of(state.getPropAsInt(key));
- } else if (new TypeToken<Long>() {}.getRawType().isAssignableFrom(token.getClass())) {
+ } else if (new TypeToken<Long>(){}.isSupertypeOf(token)) {
fieldValue = (Optional<T>) Optional.of(state.getPropAsLong(key));
- } else if (new TypeToken<List<String>>() {}.getRawType().isAssignableFrom(token.getClass())) {
Review Comment:
Yup that's what happened, shouldn't have taken the raw type unless I was also getting the raw type from the incoming token as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] phet commented on a diff in pull request #3688: [GOBBLIN-1826] Change isAssignableFrom() to isSuperTypeOf() per Guava 20 javadocs to…
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3688:
URL: https://github.com/apache/gobblin/pull/3688#discussion_r1179460218
##########
gobblin-hive-registration/src/test/java/org/apache/gobblin/hive/HiveTableTest.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.
+ */
+
+package org.apache.gobblin.hive;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.testng.annotations.Test;
+
+import junit.framework.Assert;
+
+import org.apache.gobblin.configuration.State;
+
+public class HiveTableTest {
Review Comment:
fair enough
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] codecov-commenter commented on pull request #3688: [GOBBLIN-1826] Change isAssignableFrom() to isSuperTypeOf() per Guava 20 javadocs to…
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3688:
URL: https://github.com/apache/gobblin/pull/3688#issuecomment-1524591065
## [Codecov](https://codecov.io/gh/apache/gobblin/pull/3688?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3688](https://codecov.io/gh/apache/gobblin/pull/3688?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (eadea87) into [master](https://codecov.io/gh/apache/gobblin/commit/633891080536b39f8c9c127af932d7e8b601b79e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6338910) will **decrease** coverage by `2.22%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #3688 +/- ##
============================================
- Coverage 46.97% 44.76% -2.22%
+ Complexity 10784 2091 -8693
============================================
Files 2138 411 -1727
Lines 84040 17721 -66319
Branches 9340 2159 -7181
============================================
- Hits 39480 7932 -31548
+ Misses 40969 8932 -32037
+ Partials 3591 857 -2734
```
[see 1729 files with indirect coverage changes](https://codecov.io/gh/apache/gobblin/pull/3688/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] phet commented on a diff in pull request #3688: [GOBBLIN-1826] Change isAssignableFrom() to isSuperTypeOf() per Guava 20 javadocs to…
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3688:
URL: https://github.com/apache/gobblin/pull/3688#discussion_r1178692889
##########
gobblin-hive-registration/src/main/java/org/apache/gobblin/hive/HiveRegistrationUnit.java:
##########
@@ -125,14 +125,13 @@ protected void populateSerDeFields(State state) {
protected static <T> Optional<T> populateField(State state, String key, TypeToken<T> token) {
if (state.contains(key)) {
Optional<T> fieldValue;
-
- if (new TypeToken<Boolean>() {}.getRawType().isAssignableFrom(token.getClass())) {
+ if (new TypeToken<Boolean>(){}.isSupertypeOf(token)) {
fieldValue = (Optional<T>) Optional.of(state.getPropAsBoolean(key));
- } else if (new TypeToken<Integer>() {}.getRawType().isAssignableFrom(token.getClass())) {
+ } else if (new TypeToken<Integer>(){}.isSupertypeOf(token)) {
fieldValue = (Optional<T>) Optional.of(state.getPropAsInt(key));
- } else if (new TypeToken<Long>() {}.getRawType().isAssignableFrom(token.getClass())) {
+ } else if (new TypeToken<Long>(){}.isSupertypeOf(token)) {
fieldValue = (Optional<T>) Optional.of(state.getPropAsLong(key));
- } else if (new TypeToken<List<String>>() {}.getRawType().isAssignableFrom(token.getClass())) {
Review Comment:
I believe what happened [during the recent guava 20.0 bump](https://github.com/apache/gobblin/pull/3669/files#diff-b846a4c4070e2ba285002541a89db7d267851d120a9cedc1360c4e91ae240eef) is that this became
```
x.isAssignableFrom(y)
// where:
// Class<List<?>> x
// Class<TypeToken<?>> y
```
so it just fell through to `Optional.of(state.getProp(key))` on line 137
is that your interpretation as well?
##########
gobblin-hive-registration/src/test/java/org/apache/gobblin/hive/HiveTableTest.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.
+ */
+
+package org.apache.gobblin.hive;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.testng.annotations.Test;
+
+import junit.framework.Assert;
+
+import org.apache.gobblin.configuration.State;
+
+public class HiveTableTest {
Review Comment:
wondering... could this not be a test of `HiveRegistrationUnit`, rather than its `HiveTable` derived type?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org