[GitHub] [tika] PeterAlfredLee opened a new pull request #334: Tika-3141 : add empty environment variable handle

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] PeterAlfredLee opened a new pull request #334: Tika-3141 : add empty environment variable handle

GitBox

PeterAlfredLee opened a new pull request #334:
URL: https://github.com/apache/tika/pull/334


   Trying to fix Tika-3141 with a empty string check in `TikaConfig`


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] keithrbennett commented on a change in pull request #334: Tika-3141 : add empty environment variable handle

GitBox

keithrbennett commented on a change in pull request #334:
URL: https://github.com/apache/tika/pull/334#discussion_r463103079



##########
File path: tika-core/src/main/java/org/apache/tika/config/TikaConfig.java
##########
@@ -249,11 +249,11 @@ public TikaConfig(ClassLoader loader)
     public TikaConfig() throws TikaException, IOException {
 
         String config = System.getProperty("tika.config");
-        if (config == null) {
+        if (config == null || config.trim().equals("")) {

Review comment:
       Could you use `StringUtils.blank(config)` instead? This would be more concise, and we are using StringUtils methods in other parts of the code as well. (See https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/StringUtils.html#isBlank-java.lang.CharSequence-).




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] PeterAlfredLee commented on a change in pull request #334: Tika-3141 : add empty environment variable handle

GitBox
In reply to this post by GitBox

PeterAlfredLee commented on a change in pull request #334:
URL: https://github.com/apache/tika/pull/334#discussion_r463905876



##########
File path: tika-core/src/main/java/org/apache/tika/config/TikaConfig.java
##########
@@ -249,11 +249,11 @@ public TikaConfig(ClassLoader loader)
     public TikaConfig() throws TikaException, IOException {
 
         String config = System.getProperty("tika.config");
-        if (config == null) {
+        if (config == null || config.trim().equals("")) {

Review comment:
       It seems the `StringUtils` is part of `Apache Commons Lang`, which is not a dependecy of `tika core`. Should I add it in the `pom.xml`?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] PeterAlfredLee commented on a change in pull request #334: Tika-3141 : add empty environment variable handle

GitBox
In reply to this post by GitBox

PeterAlfredLee commented on a change in pull request #334:
URL: https://github.com/apache/tika/pull/334#discussion_r463906635



##########
File path: tika-core/src/main/java/org/apache/tika/config/TikaConfig.java
##########
@@ -249,11 +249,11 @@ public TikaConfig(ClassLoader loader)
     public TikaConfig() throws TikaException, IOException {
 
         String config = System.getProperty("tika.config");
-        if (config == null) {
+        if (config == null || config.trim().equals("")) {

Review comment:
       And I think we should add a log message here for `config` that is not null but empty. WDYT?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] keithrbennett commented on a change in pull request #334: Tika-3141 : add empty environment variable handle

GitBox
In reply to this post by GitBox

keithrbennett commented on a change in pull request #334:
URL: https://github.com/apache/tika/pull/334#discussion_r463971330



##########
File path: tika-core/src/main/java/org/apache/tika/config/TikaConfig.java
##########
@@ -249,11 +249,11 @@ public TikaConfig(ClassLoader loader)
     public TikaConfig() throws TikaException, IOException {
 
         String config = System.getProperty("tika.config");
-        if (config == null) {
+        if (config == null || config.trim().equals("")) {

Review comment:
       > It seems the `StringUtils` is part of `Apache Commons Lang`, which is not a dependecy of `tika core`. Should I add it in the `pom.xml`?
   
   Since it is a dependency of other Tika modules:
   
   ```
   % grep "commons-lang" **/pom.xml
   tika-bundle/pom.xml:              commons-lang3|
   tika-dl/pom.xml:          <artifactId>commons-lang3</artifactId>
   tika-dl/pom.xml:          <artifactId>commons-lang3</artifactId>
   tika-dl/pom.xml:          <groupId>commons-lang</groupId>
   tika-dl/pom.xml:          <artifactId>commons-lang</artifactId>
   tika-eval/pom.xml:            <artifactId>commons-lang3</artifactId>
   tika-nlp/pom.xml:          <groupId>commons-lang</groupId>
   tika-nlp/pom.xml:          <artifactId>commons-lang</artifactId>
   tika-parsers/pom.xml:      <artifactId>commons-lang3</artifactId>
   tika-parsers/pom.xml:          <artifactId>commons-lang3</artifactId>
   tika-parsers/pom.xml:          <artifactId>commons-lang3</artifactId>
   tika-server/pom.xml:      <artifactId>commons-lang3</artifactId>
   ```
   
   ...if you don't hear from anyone soon II would suggest adding it to tika-core and let the person authorizing the pull request merge make that call. I suspect it's ok, because it's used elsewhere in Tika.
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] keithrbennett commented on a change in pull request #334: Tika-3141 : add empty environment variable handle

GitBox
In reply to this post by GitBox

keithrbennett commented on a change in pull request #334:
URL: https://github.com/apache/tika/pull/334#discussion_r463971330



##########
File path: tika-core/src/main/java/org/apache/tika/config/TikaConfig.java
##########
@@ -249,11 +249,11 @@ public TikaConfig(ClassLoader loader)
     public TikaConfig() throws TikaException, IOException {
 
         String config = System.getProperty("tika.config");
-        if (config == null) {
+        if (config == null || config.trim().equals("")) {

Review comment:
       > It seems the `StringUtils` is part of `Apache Commons Lang`, which is not a dependecy of `tika core`. Should I add it in the `pom.xml`?
   
   Since it is a dependency of other Tika modules:
   
   ```
   % grep "commons-lang" **/pom.xml
   tika-bundle/pom.xml:              commons-lang3|
   tika-dl/pom.xml:          <artifactId>commons-lang3</artifactId>
   tika-dl/pom.xml:          <artifactId>commons-lang3</artifactId>
   tika-dl/pom.xml:          <groupId>commons-lang</groupId>
   tika-dl/pom.xml:          <artifactId>commons-lang</artifactId>
   tika-eval/pom.xml:            <artifactId>commons-lang3</artifactId>
   tika-nlp/pom.xml:          <groupId>commons-lang</groupId>
   tika-nlp/pom.xml:          <artifactId>commons-lang</artifactId>
   tika-parsers/pom.xml:      <artifactId>commons-lang3</artifactId>
   tika-parsers/pom.xml:          <artifactId>commons-lang3</artifactId>
   tika-parsers/pom.xml:          <artifactId>commons-lang3</artifactId>
   tika-server/pom.xml:      <artifactId>commons-lang3</artifactId>
   ```
   
   ...if you don't hear from anyone soon I would suggest adding it to tika-core and let the person authorizing the pull request merge make that call. I suspect it's ok, because it's used elsewhere in Tika.
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] PeterAlfredLee commented on a change in pull request #334: Tika-3141 : add empty environment variable handle

GitBox
In reply to this post by GitBox

PeterAlfredLee commented on a change in pull request #334:
URL: https://github.com/apache/tika/pull/334#discussion_r464218911



##########
File path: tika-core/src/main/java/org/apache/tika/config/TikaConfig.java
##########
@@ -249,11 +249,11 @@ public TikaConfig(ClassLoader loader)
     public TikaConfig() throws TikaException, IOException {
 
         String config = System.getProperty("tika.config");
-        if (config == null) {
+        if (config == null || config.trim().equals("")) {

Review comment:
       Sure. Maybe we should have it discussed in mailing list.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]