Bug 61700

Summary: [PATCH] getForceFormulaRecalculation() returns wrong value
Product: POI Reporter: jchezenko
Component: XSSFAssignee: POI Developers List <dev>
Status: RESOLVED FIXED    
Severity: normal CC: nospam
Priority: P2 Keywords: PatchAvailable
Version: 3.17-FINAL   
Target Milestone: ---   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 61863, 61864    
Attachments: patch against current trunk with the suggested changes and the tescase commented above

Description jchezenko 2017-10-31 00:47:56 UTC
I think getForceFormulaRecalculation is implemented wrong.


Some logging code in my applicaiton
>>>
boolean forceFormulaRecalculation = workbook.getForceFormulaRecalculation();
log.error("Will excel recalculate? {}", forceFormulaRecalculation);
workbook.setForceFormulaRecalculation(true);
boolean forceFormulaRecalculation2 = workbook.getForceFormulaRecalculation();
>>>

Expected Behavior
- Will excel recalculate? false
- Will excel recalculate after setting to true? true


Actual behavior
- Will excel recalculate? true
- Will excel recalculate after setting to true? false


Shouldn't this line of code here be 

return calcPr != null && calcPr.getCalcId() == 0;

https://github.com/apache/poi/blob/REL_3_17_FINAL/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFWorkbook.java#L2232



Additional Context:
XSSF workbook
Comment 1 Javen O'Neal 2017-10-31 09:18:44 UTC
https://poi.apache.org/spreadsheet/eval.html#Recalculation+of+Formulas

@Test
public void testWorkbookForceFormulaRecalculation() throws Exception {
    Workbook workbook = _testDataProvider.createWorkbook();
    workbook.createSheet().createRow(0).createCell(0).setCellFormula("B1+C1");
    workbook.getCreationHelper().createFormulaEvaluator().evaluateAll();

    assertFalse(workbook.getForceFormulaRecalculation());
    workbook.setForceFormulaRecalculation(true);
    assertTrue(workbook.getForceFormulaRecalculation());

    Workbook wbBack = _testDataProvider.writeOutAndReadBack(workbook);
    assertTrue(wbBack.getForceFormulaRecalculation());

    workbook.close();
    wbBack.close();
}
Comment 2 Kai G 2019-03-22 09:20:43 UTC
Created attachment 36494 [details]
patch against current trunk with the suggested changes and the tescase commented above

I've added a patch including the testcase referenced above
Comment 3 PJ Fanning 2019-03-31 00:00:20 UTC
patch applied with https://svn.apache.org/viewvc?view=revision&revision=1856650
Comment 4 Greg Woolsey 2019-03-31 00:22:56 UTC
Comment on attachment 36494 [details]
patch against current trunk with the suggested changes and the tescase commented above

attempt to fix attachment MIME type
Comment 5 Greg Woolsey 2019-03-31 00:24:02 UTC
Comment on attachment 36494 [details]
patch against current trunk with the suggested changes and the tescase commented above

fix attachment MIME type
Comment 6 Greg Woolsey 2019-03-31 01:12:59 UTC
see r1856652

From the OOXML spec, using calcId is a hack, and what really should be used is

fullCalcOnLoad=1

calcId=0 just means "your engine is newer, recalculate everything when opening" while fullCalcOnLoad explicitly says what we want done.

Yes, I think you are right about the logic of that line, but really it should be looking for the other property, not the calcId=0 hack.  

OOXML specifies a default value of "false" for fullCalcOnLoad, which I think we can assume also applies when the attribute or calcPr entity are missing.

POI should leave the engine ID alone and use the actual boolean attribute instead. 

Per this thread [0] Excel and everything else honor this flag.

[0] https://stackoverflow.com/questions/18355691/set-xlsx-to-recalculate-formulae-on-open
Comment 7 Greg Woolsey 2019-03-31 01:14:14 UTC
further, XSSFSheet.setForceFormulaRecalculation() already uses the fullCalcOnLoad property.  Mentioning for completeness.