fix an NPE in Spreadsheet.createCell() when the cell value is null #607
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Reviewed 2 of 2 files at r1. Comments from Reviewable |
|
Reviewed 1 of 2 files at r1. Comments from Reviewable |
DiegoCardoso
pushed a commit
that referenced
this pull request
Sep 7, 2017
* Fixes for #461 this change addresses all but #4 in that issue (no changing default cell border CSS), and gets things ready to add table style support as well (#529), by introducing the IncrementalStyleBuilder class to break out common Excel to CSS style conversion code. * Fixes for #461 add null check to satisfy unit test * Fixes for #461 not exactly sure why, but switching order of operations fixes a bunch of tests for me. My test setup isn't exactly the same, though, so this is as much to see how much closer TeamCity says this is to passing. * Fixes for #461 added missing Serializable interface to 1 class. Stop creating new sheet rows in the middle of iterating on sheet rows, causing ConcurrentModificationException. Didn't need Cell/Row objects, only CellReference objects in that context, which don't have to refer to existing cells. Let's see how much closer this gets to passing all tests. * Fixes for #461 treat cells with unsupported functions in their formula evaluation chain as not matching conditional formatting, and don't propagate the error. May want to add logging eventually, although it is flagged elsewhere already. * fix an NPE in Spreadsheet.createCell() when the cell value is null (#607) fixes #603 * Fixes for #461 found that the previous implementation used an implicit null return value to indicate no conditional formats applied to a cell (undocumented, bad programming form to use null as a semantic element). Had to widen the check to include an empty Set<Integer> as well as atomically null, since the new implementation always returns a non-null Set. I see that all cells with conditional format styles applied are marked for update and sent to the client on every change. That is likely fine, most workbooks don't have that many, but really it should check and update only on incremental changes. Also, it didn't notice when conditional format(s) no longer applied to a cell, and didn't update it to have no formatting in those cases, so ConditionalFormatter marks all cells where the formatting changed from the last time it was checked. * Fixes for #461 Optimization and bug fix to avoid repeatedly evaluating conditional formatting for the same cell when making a full pass. The only times getCellFormattingIndex(Cell) is called are when, at the end of a server response, cells are iterated over to see what needs updating. When doing that, border rules require evaluating neighbor cells for each cell, causing duplicate work. This tracks cells already evaluated in that run, and re-uses their results rather than recalculating them, while still clearing the results for the next run to pick up changes to formula evaluations which can change formatting. * Fixes for #461 Rework cell conditional formatting evaluation caching to encapsulate it. Added JavaDoc comments to uncached methods indicating that fact. * Fix for #461 Fix javadoc * Fixes for #461 Make the interfaces serializable.
alexberazouski
pushed a commit
that referenced
this pull request
Dec 7, 2017
* Fixes for #461 this change addresses all but #4 in that issue (no changing default cell border CSS), and gets things ready to add table style support as well (#529), by introducing the IncrementalStyleBuilder class to break out common Excel to CSS style conversion code. * Fixes for #461 add null check to satisfy unit test * Fixes for #461 not exactly sure why, but switching order of operations fixes a bunch of tests for me. My test setup isn't exactly the same, though, so this is as much to see how much closer TeamCity says this is to passing. * Fixes for #461 added missing Serializable interface to 1 class. Stop creating new sheet rows in the middle of iterating on sheet rows, causing ConcurrentModificationException. Didn't need Cell/Row objects, only CellReference objects in that context, which don't have to refer to existing cells. Let's see how much closer this gets to passing all tests. * Fixes for #461 treat cells with unsupported functions in their formula evaluation chain as not matching conditional formatting, and don't propagate the error. May want to add logging eventually, although it is flagged elsewhere already. * fix an NPE in Spreadsheet.createCell() when the cell value is null (#607) fixes #603 * Fixes for #461 found that the previous implementation used an implicit null return value to indicate no conditional formats applied to a cell (undocumented, bad programming form to use null as a semantic element). Had to widen the check to include an empty Set<Integer> as well as atomically null, since the new implementation always returns a non-null Set. I see that all cells with conditional format styles applied are marked for update and sent to the client on every change. That is likely fine, most workbooks don't have that many, but really it should check and update only on incremental changes. Also, it didn't notice when conditional format(s) no longer applied to a cell, and didn't update it to have no formatting in those cases, so ConditionalFormatter marks all cells where the formatting changed from the last time it was checked. * Fixes for #461 Optimization and bug fix to avoid repeatedly evaluating conditional formatting for the same cell when making a full pass. The only times getCellFormattingIndex(Cell) is called are when, at the end of a server response, cells are iterated over to see what needs updating. When doing that, border rules require evaluating neighbor cells for each cell, causing duplicate work. This tracks cells already evaluated in that run, and re-uses their results rather than recalculating them, while still clearing the results for the next run to pick up changes to formula evaluations which can change formatting. * Fixes for #461 Rework cell conditional formatting evaluation caching to encapsulate it. Added JavaDoc comments to uncached methods indicating that fact. * Fix for #461 Fix javadoc * Fixes for #461 Make the interfaces serializable.
alexberazouski
pushed a commit
that referenced
this pull request
Dec 8, 2017
* upgrade the version of POI to 3.17-beta1 * ignore a test failing due to a (known) POI issue https://bz.apache.org/bugzilla/show_bug.cgi?id=61468 * Ignore some conditional test due to POI errors This bug is preventing PR #595 to pass and it is expected to be fixed when POI 3.17 is finalized. Then, these tests should be enabled again. More info at #595 (comment) * Fixes for #461 (#595) * Fixes for #461 this change addresses all but #4 in that issue (no changing default cell border CSS), and gets things ready to add table style support as well (#529), by introducing the IncrementalStyleBuilder class to break out common Excel to CSS style conversion code. * Fixes for #461 add null check to satisfy unit test * Fixes for #461 not exactly sure why, but switching order of operations fixes a bunch of tests for me. My test setup isn't exactly the same, though, so this is as much to see how much closer TeamCity says this is to passing. * Fixes for #461 added missing Serializable interface to 1 class. Stop creating new sheet rows in the middle of iterating on sheet rows, causing ConcurrentModificationException. Didn't need Cell/Row objects, only CellReference objects in that context, which don't have to refer to existing cells. Let's see how much closer this gets to passing all tests. * Fixes for #461 treat cells with unsupported functions in their formula evaluation chain as not matching conditional formatting, and don't propagate the error. May want to add logging eventually, although it is flagged elsewhere already. * fix an NPE in Spreadsheet.createCell() when the cell value is null (#607) fixes #603 * Fixes for #461 found that the previous implementation used an implicit null return value to indicate no conditional formats applied to a cell (undocumented, bad programming form to use null as a semantic element). Had to widen the check to include an empty Set<Integer> as well as atomically null, since the new implementation always returns a non-null Set. I see that all cells with conditional format styles applied are marked for update and sent to the client on every change. That is likely fine, most workbooks don't have that many, but really it should check and update only on incremental changes. Also, it didn't notice when conditional format(s) no longer applied to a cell, and didn't update it to have no formatting in those cases, so ConditionalFormatter marks all cells where the formatting changed from the last time it was checked. * Fixes for #461 Optimization and bug fix to avoid repeatedly evaluating conditional formatting for the same cell when making a full pass. The only times getCellFormattingIndex(Cell) is called are when, at the end of a server response, cells are iterated over to see what needs updating. When doing that, border rules require evaluating neighbor cells for each cell, causing duplicate work. This tracks cells already evaluated in that run, and re-uses their results rather than recalculating them, while still clearing the results for the next run to pick up changes to formula evaluations which can change formatting. * Fixes for #461 Rework cell conditional formatting evaluation caching to encapsulate it. Added JavaDoc comments to uncached methods indicating that fact. * Fix for #461 Fix javadoc * Fixes for #461 Make the interfaces serializable. * Update POI to 3.17 release version. Removed @ignore on tests that should be working in this version of POI. * Ignore conditional formatter tests After update POI to 3.17 there is some regression in the following tests: ConditionalFormattingBasedOnFormulaTest.loadSpreadsheetWithConditionalFormattingRulesInRow10_EvaluateFormatting_CheckColorOfCells ConditionalFormattingBasedOnSharedFormulaTest.loadSpreadsheetWithConditionalFormattingInA1A2_A3B4_D1G5___CheckCellFormatting ConditionalFormattingCellValueIsTest.loadSpreadsheetWithNotEqualConditionFormattingInB4_insertIncoherentValue_CellB4FilledRed * Fix formatting, organize imports
tulioag
pushed a commit
that referenced
this pull request
Aug 28, 2018
* upgrade the version of POI to 3.17-beta1 * ignore a test failing due to a (known) POI issue https://bz.apache.org/bugzilla/show_bug.cgi?id=61468 * Ignore some conditional test due to POI errors This bug is preventing PR #595 to pass and it is expected to be fixed when POI 3.17 is finalized. Then, these tests should be enabled again. More info at #595 (comment) * Fixes for #461 (#595) * Fixes for #461 this change addresses all but #4 in that issue (no changing default cell border CSS), and gets things ready to add table style support as well (#529), by introducing the IncrementalStyleBuilder class to break out common Excel to CSS style conversion code. * Fixes for #461 add null check to satisfy unit test * Fixes for #461 not exactly sure why, but switching order of operations fixes a bunch of tests for me. My test setup isn't exactly the same, though, so this is as much to see how much closer TeamCity says this is to passing. * Fixes for #461 added missing Serializable interface to 1 class. Stop creating new sheet rows in the middle of iterating on sheet rows, causing ConcurrentModificationException. Didn't need Cell/Row objects, only CellReference objects in that context, which don't have to refer to existing cells. Let's see how much closer this gets to passing all tests. * Fixes for #461 treat cells with unsupported functions in their formula evaluation chain as not matching conditional formatting, and don't propagate the error. May want to add logging eventually, although it is flagged elsewhere already. * fix an NPE in Spreadsheet.createCell() when the cell value is null (#607) fixes #603 * Fixes for #461 found that the previous implementation used an implicit null return value to indicate no conditional formats applied to a cell (undocumented, bad programming form to use null as a semantic element). Had to widen the check to include an empty Set<Integer> as well as atomically null, since the new implementation always returns a non-null Set. I see that all cells with conditional format styles applied are marked for update and sent to the client on every change. That is likely fine, most workbooks don't have that many, but really it should check and update only on incremental changes. Also, it didn't notice when conditional format(s) no longer applied to a cell, and didn't update it to have no formatting in those cases, so ConditionalFormatter marks all cells where the formatting changed from the last time it was checked. * Fixes for #461 Optimization and bug fix to avoid repeatedly evaluating conditional formatting for the same cell when making a full pass. The only times getCellFormattingIndex(Cell) is called are when, at the end of a server response, cells are iterated over to see what needs updating. When doing that, border rules require evaluating neighbor cells for each cell, causing duplicate work. This tracks cells already evaluated in that run, and re-uses their results rather than recalculating them, while still clearing the results for the next run to pick up changes to formula evaluations which can change formatting. * Fixes for #461 Rework cell conditional formatting evaluation caching to encapsulate it. Added JavaDoc comments to uncached methods indicating that fact. * Fix for #461 Fix javadoc * Fixes for #461 Make the interfaces serializable. * Update POI to 3.17 release version. Removed @ignore on tests that should be working in this version of POI. * Ignore conditional formatter tests After update POI to 3.17 there is some regression in the following tests: ConditionalFormattingBasedOnFormulaTest.loadSpreadsheetWithConditionalFormattingRulesInRow10_EvaluateFormatting_CheckColorOfCells ConditionalFormattingBasedOnSharedFormulaTest.loadSpreadsheetWithConditionalFormattingInA1A2_A3B4_D1G5___CheckCellFormatting ConditionalFormattingCellValueIsTest.loadSpreadsheetWithNotEqualConditionFormattingInB4_insertIncoherentValue_CellB4FilledRed * Fix formatting, organize imports (cherry picked from commit ed2822c) # Conflicts: # vaadin-spreadsheet/pom.xml # vaadin-spreadsheet/src/main/java/com/vaadin/addon/spreadsheet/XSSFColorConverter.java
tulioag
pushed a commit
that referenced
this pull request
Aug 29, 2018
* Update POI to 3.17 (#648) * upgrade the version of POI to 3.17-beta1 * ignore a test failing due to a (known) POI issue https://bz.apache.org/bugzilla/show_bug.cgi?id=61468 * Ignore some conditional test due to POI errors This bug is preventing PR #595 to pass and it is expected to be fixed when POI 3.17 is finalized. Then, these tests should be enabled again. More info at #595 (comment) * Fixes for #461 (#595) * Fixes for #461 this change addresses all but #4 in that issue (no changing default cell border CSS), and gets things ready to add table style support as well (#529), by introducing the IncrementalStyleBuilder class to break out common Excel to CSS style conversion code. * Fixes for #461 add null check to satisfy unit test * Fixes for #461 not exactly sure why, but switching order of operations fixes a bunch of tests for me. My test setup isn't exactly the same, though, so this is as much to see how much closer TeamCity says this is to passing. * Fixes for #461 added missing Serializable interface to 1 class. Stop creating new sheet rows in the middle of iterating on sheet rows, causing ConcurrentModificationException. Didn't need Cell/Row objects, only CellReference objects in that context, which don't have to refer to existing cells. Let's see how much closer this gets to passing all tests. * Fixes for #461 treat cells with unsupported functions in their formula evaluation chain as not matching conditional formatting, and don't propagate the error. May want to add logging eventually, although it is flagged elsewhere already. * fix an NPE in Spreadsheet.createCell() when the cell value is null (#607) fixes #603 * Fixes for #461 found that the previous implementation used an implicit null return value to indicate no conditional formats applied to a cell (undocumented, bad programming form to use null as a semantic element). Had to widen the check to include an empty Set<Integer> as well as atomically null, since the new implementation always returns a non-null Set. I see that all cells with conditional format styles applied are marked for update and sent to the client on every change. That is likely fine, most workbooks don't have that many, but really it should check and update only on incremental changes. Also, it didn't notice when conditional format(s) no longer applied to a cell, and didn't update it to have no formatting in those cases, so ConditionalFormatter marks all cells where the formatting changed from the last time it was checked. * Fixes for #461 Optimization and bug fix to avoid repeatedly evaluating conditional formatting for the same cell when making a full pass. The only times getCellFormattingIndex(Cell) is called are when, at the end of a server response, cells are iterated over to see what needs updating. When doing that, border rules require evaluating neighbor cells for each cell, causing duplicate work. This tracks cells already evaluated in that run, and re-uses their results rather than recalculating them, while still clearing the results for the next run to pick up changes to formula evaluations which can change formatting. * Fixes for #461 Rework cell conditional formatting evaluation caching to encapsulate it. Added JavaDoc comments to uncached methods indicating that fact. * Fix for #461 Fix javadoc * Fixes for #461 Make the interfaces serializable. * Update POI to 3.17 release version. Removed @ignore on tests that should be working in this version of POI. * Ignore conditional formatter tests After update POI to 3.17 there is some regression in the following tests: ConditionalFormattingBasedOnFormulaTest.loadSpreadsheetWithConditionalFormattingRulesInRow10_EvaluateFormatting_CheckColorOfCells ConditionalFormattingBasedOnSharedFormulaTest.loadSpreadsheetWithConditionalFormattingInA1A2_A3B4_D1G5___CheckCellFormatting ConditionalFormattingCellValueIsTest.loadSpreadsheetWithNotEqualConditionFormattingInB4_insertIncoherentValue_CellB4FilledRed * Fix formatting, organize imports (cherry picked from commit ed2822c) # Conflicts: # vaadin-spreadsheet/pom.xml # vaadin-spreadsheet/src/main/java/com/vaadin/addon/spreadsheet/XSSFColorConverter.java * Ignored org.apache.felix in Eclipse * Removed java 8 annotations. * Bumped version to next minor.
manolo
pushed a commit
to vaadin/flow-components
that referenced
this pull request
Apr 27, 2022
manolo
pushed a commit
to vaadin/flow-components
that referenced
this pull request
Apr 27, 2022
vaadin/spreadsheet#648 * upgrade the version of POI to 3.17-beta1 * ignore a test failing due to a (known) POI issue https://bz.apache.org/bugzilla/show_bug.cgi?id=61468 * Ignore some conditional test due to POI errors This bug is preventing PRvaadin/spreadsheet#595 to pass and it is expected to be fixed when POI 3.17 is finalized. Then, these tests should be enabled again. More info at vaadin/spreadsheet#595 (comment) * Fixes forvaadin/spreadsheet#461 vaadin/spreadsheet#595) * Fixes for vaadin/spreadsheet#461 this change addresses all butvaadin/spreadsheet#4 in that issue (no changing default cell border CSS), and gets things ready to add table style support as well vaadin/spreadsheet#529), by introducing the IncrementalStyleBuilder class to break out common Excel to CSS style conversion code. * Fixes forvaadin/spreadsheet#461 add null check to satisfy unit test * Fixes forvaadin/spreadsheet#461 not exactly sure why, but switching order of operations fixes a bunch of tests for me. My test setup isn't exactly the same, though, so this is as much to see how much closer TeamCity says this is to passing. * Fixes forvaadin/spreadsheet#461 added missing Serializable interface to 1 class. Stop creating new sheet rows in the middle of iterating on sheet rows, causing ConcurrentModificationException. Didn't need Cell/Row objects, only CellReference objects in that context, which don't have to refer to existing cells. Let's see how much closer this gets to passing all tests. * Fixes forvaadin/spreadsheet#461 treat cells with unsupported functions in their formula evaluation chain as not matching conditional formatting, and don't propagate the error. May want to add logging eventually, although it is flagged elsewhere already. * fix an NPE in Spreadsheet.createCell() when the cell value is null vaadin/spreadsheet#607) Fixes: vaadin/spreadsheet#603 * Fixes forvaadin/spreadsheet#461 found that the previous implementation used an implicit null return value to indicate no conditional formats applied to a cell (undocumented, bad programming form to use null as a semantic element). Had to widen the check to include an empty Set<Integer> as well as atomically null, since the new implementation always returns a non-null Set. I see that all cells with conditional format styles applied are marked for update and sent to the client on every change. That is likely fine, most workbooks don't have that many, but really it should check and update only on incremental changes. Also, it didn't notice when conditional format(s) no longer applied to a cell, and didn't update it to have no formatting in those cases, so ConditionalFormatter marks all cells where the formatting changed from the last time it was checked. * Fixes forvaadin/spreadsheet#461 Optimization and bug fix to avoid repeatedly evaluating conditional formatting for the same cell when making a full pass. The only times getCellFormattingIndex(Cell) is called are when, at the end of a server response, cells are iterated over to see what needs updating. When doing that, border rules require evaluating neighbor cells for each cell, causing duplicate work. This tracks cells already evaluated in that run, and re-uses their results rather than recalculating them, while still clearing the results for the next run to pick up changes to formula evaluations which can change formatting. * Fixes forvaadin/spreadsheet#461 Rework cell conditional formatting evaluation caching to encapsulate it. Added JavaDoc comments to uncached methods indicating that fact. * Fix forvaadin/spreadsheet#461 Fix javadoc * Fixes forvaadin/spreadsheet#461 Make the interfaces serializable. * Update POI to 3.17 release version. Removed @ignore on tests that should be working in this version of POI. * Ignore conditional formatter tests After update POI to 3.17 there is some regression in the following tests: ConditionalFormattingBasedOnFormulaTest.loadSpreadsheetWithConditionalFormattingRulesInRow10_EvaluateFormatting_CheckColorOfCells ConditionalFormattingBasedOnSharedFormulaTest.loadSpreadsheetWithConditionalFormattingInA1A2_A3B4_D1G5___CheckCellFormatting ConditionalFormattingCellValueIsTest.loadSpreadsheetWithNotEqualConditionFormattingInB4_insertIncoherentValue_CellB4FilledRed * Fix formatting, organize imports
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #603
This change is