Skip to content

Use IOException instead of IOExceptionWithCause#369

Merged
PeterAlfredLee merged 1 commit intoapache:mainfrom
PeterAlfredLee:useIOException
Jan 14, 2021
Merged

Use IOException instead of IOExceptionWithCause#369
PeterAlfredLee merged 1 commit intoapache:mainfrom
PeterAlfredLee:useIOException

Conversation

@PeterAlfredLee
Copy link
Copy Markdown
Member

We used to use IOExceptionWithCause because IOException with the Throwable constructors is missing before JDK 6.
I think we should use IOException after JDK 6.

Copy link
Copy Markdown
Contributor

@kkrugler kkrugler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the IOExceptionWithCause class being deprecated - I assume we'd keep it around until maybe 2.0, as removing it would be a breaking change.

@PeterAlfredLee
Copy link
Copy Markdown
Member Author

@kkrugler Thank you for reviewing this.

I don't see the IOExceptionWithCause class being deprecated

IOExceptionWithCause in Tika was copying from commons-io 1.4.
And IOExceptionWithCause in commons-io has be deprecated since commons-io 2.5. see this

as removing it would be a breaking change

Agree, This will break binary compatibility. And we should only break BC in major version.
As I see, main branch is prepare for Tika-2.0 which will be the new major version. So I believe this is a good time to break BC.

@kkrugler
Copy link
Copy Markdown
Contributor

If this change is only going into main, then my question is whether you removed the IOExceptionWithCause class from Tika (maybe I missed that), and if not, why not?

@PeterAlfredLee
Copy link
Copy Markdown
Member Author

I'm also agree to remove the IOExceptionWithCause class from Tika.
The reason why this PR wasn't removing IOExceptionWithCause class is because consider TaggedIOException class in Tika need IOExceptionWithCause class.

TaggedIOException class in Tika was copying from commons-io 1.4. So I perfer not to modify TaggedIOException class.

By the way,I think we can remove all these class from Tika which was copying from commons-io 1.4 if we make commons-io as dependency of tika-core.

WDYT ?

@kkrugler
Copy link
Copy Markdown
Contributor

TaggedIOException can just extend IOException, yes?

As to adding a dependency on commons-io, that's been discussed in the past, but the decision at that time was to not add the dependency, to keep tika-core as light-weight as possible. I'd take a look at the mailing list archives to get the full context of that discussion (I think there might also be Jira issues), if you want to revisit it.

@PeterAlfredLee
Copy link
Copy Markdown
Member Author

TaggedIOException can just extend IOException, yes?

Yes, it is.

I'd take a look at the mailing list archives to get the full context of that discussion (I think there might also be Jira issues), if you want to revisit it.

Maybe you are talking about TIKA-1706 and TIKA-1710? Seems that it's agreed to have commons-io as a dependency of tika-core. Are there any other mailing threads or issues? It's appreciated if you can find them. :)

@tballison
Copy link
Copy Markdown
Contributor

My memory was that we had agreed to add commons-io to tika-core in 2.x, and Bob already did that in the 2.x branch, I haven't had a chance to port those updates to main yet.

I, frankly, want to keep some subclass of IOException around whether that's IOExceptionWithCause or something else. My reasoning is that we have to wrap SAXExceptions and TikaExceptions in IOExceptions because of the exception signatures in some of our overridden classes. It is useful to be able differentiate this hack from an actual IOException.

Is there a better way to achieve this goal?

@PeterAlfredLee
Copy link
Copy Markdown
Member Author

My memory was that we had agreed to add commons-io to tika-core in 2.x, and Bob already did that in the 2.x branch, I haven't had a chance to port those updates to main yet.

I see. I'm almost finished with the code and tests. Maybe I can push a PR to main branch?

Is there a better way to achieve this goal?

Seems we do not have any other ways to do that. Maybe you have any better ideas?

@kkrugler
Copy link
Copy Markdown
Contributor

kkrugler commented Nov 5, 2020

Hi @tballison - you said:

I, frankly, want to keep some subclass of IOException around whether that's IOExceptionWithCause or something else. My reasoning is that we have to wrap SAXExceptions and TikaExceptions in IOExceptions because of the exception signatures in some of our overridden classes. It is useful to be able differentiate this hack from an actual IOException.

Couldn't we have a helper class that checks for IOException.getCause() being an instance of those two exceptions? I'm asking because I also run across a lot of cases in other projects where the root cause has to be wrapped in an IOException, but in the very rare case where I care about what really caused the problem, I call getCause(). But maybe there's something different about this particular use case.

@kkrugler
Copy link
Copy Markdown
Contributor

Hi @tballison - hoping you can weigh in on whether we really need to keep around IOExceptionWithCause, thanks.

@tballison
Copy link
Copy Markdown
Contributor

Sorry for my delay. I'm now +1 to this.

@PeterAlfredLee
Copy link
Copy Markdown
Member Author

Hey folks. I'm thinking about merging this before the release of Tika 2.0. WDYT?

@tballison
Copy link
Copy Markdown
Contributor

+1

@kkrugler
Copy link
Copy Markdown
Contributor

@PeterAlfredLee - I thought, based on @tballison input above, that you would delete the IOExceptionWithCause class. Did that happen separately?

@PeterAlfredLee
Copy link
Copy Markdown
Member Author

PeterAlfredLee commented Dec 29, 2020

that you would delete the IOExceptionWithCause class. Did that happen separately?

Yes, this is already done in #370.

@PeterAlfredLee PeterAlfredLee merged commit 514c4a7 into apache:main Jan 14, 2021
@PeterAlfredLee PeterAlfredLee deleted the useIOException branch January 14, 2021 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants