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

Fix issues reported by findbugs #153

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

praste
Copy link

@praste praste commented Jan 31, 2017

No description provided.

Copy link
Contributor

@janhoy janhoy left a comment

Choose a reason for hiding this comment

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

Thanks @praste for your PR. Looks like you spent some time on this. Reason it has not got attention is probably because it only lives in GitHub, but we use JIRA as main issue tracker. Also we normally don't commit big common changes to unrelated classes like in this one, so I'll not merge it for you.

I'll encourage you to split this up in one JIRA issue per distinct bug, and create one PR for each of those, starting the PR title with the JIRA code, e.g. "SOLR-1234 Risk of NPE in component foo".

I can see some potentially real bugs in your list of fixes so this is valuable feedback!

@@ -357,7 +357,7 @@ public void writeArray(String name, Iterator val) throws IOException {
if (v instanceof IndexableField) {
IndexableField f = (IndexableField)v;
if (v instanceof Date) {
output.append(((Date) val).toInstant().toString() + "; ");
output.append(((Date) v).toInstant().toString() + "; ");
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks legitimate - Would cause a classCastException. But have we ever seen it in the wild?

Copy link
Author

Choose a reason for hiding this comment

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

I have not seen this in the logs.

I hit some weird error a few years back and I couldn’t believe how that we have such blatant bugs in the code all over that I ran findbugs and started wack-a-mole game for all the bugs reported.

I don’t remember what the original bug was anymore. I will give this a second shot and I will put small patches for each type of error reported by findbugs

markrmiller added a commit that referenced this pull request Jul 14, 2020
epugh pushed a commit to epugh/lucene-solr-1 that referenced this pull request Sep 3, 2021
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