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

OpenXLSX::XLDocument is not saving the same string into another workbook #283

Closed
NK01 opened this issue Oct 13, 2024 · 10 comments
Closed

OpenXLSX::XLDocument is not saving the same string into another workbook #283

NK01 opened this issue Oct 13, 2024 · 10 comments
Assignees
Labels
bug Something isn't working resolved This issue has been resolved.

Comments

@NK01
Copy link

NK01 commented Oct 13, 2024

The issue example:

OpenXLSX::XLDocument doc; // this is used to create and save the workbook
std::vector<std::vector<std::string>> data; // this contains the data to be saved in 2D matrix for easy insertion into excel

doc.create(workbook1, true);
OpenXLSX::XLWorksheet wks = doc.workbook().worksheet("Sheet1");
// Inserting data into the workbook
for (std::size_t i = 0; i < data.size(); ++i) 
{
    for (std::size_t j = 0; j < data[i].size(); ++j)
    {
        wks.cell(OpenXLSX::XLCellReference(static_cast<uint16_t>(i) + 1, static_cast<uint16_t>(j) + 1)).value() = data[i][j];
    }
}
doc.save()
doc.close()

doc.create(workbook2, true);
wks = doc.workbook().worksheet("Sheet1");
// Inserting data into the workbook
for (std::size_t i = 0; i < data.size(); ++i) 
{
    for (std::size_t j = 0; j < data[i].size(); ++j)
    {
        wks.cell(OpenXLSX::XLCellReference(static_cast<uint16_t>(i) + 1, static_cast<uint16_t>(j) + 1)).value() = data[i][j];
    }
}
doc.save()
doc.close()

In the above example the second workbook does not contain any data, but if I change some data in the std::vector<std::vector<std::string>> data matrix then the changed data is inserted into the second matrix but not anything else

Currently the workaround is to create a new OpenXLSX::XLDocument doc like this:

OpenXLSX::XLDocument doc; // this is used to create and save the workbook
std::vector<std::vector<std::string>> data; // this contains the data to be saved in 2D matrix for easy insertion into excel

doc.create(workbook1, true);
OpenXLSX::XLWorksheet wks = doc.workbook().worksheet("Sheet1");
// Inserting data into the workbook
for (std::size_t i = 0; i < data.size(); ++i) 
{
    for (std::size_t j = 0; j < data[i].size(); ++j)
    {
        wks.cell(OpenXLSX::XLCellReference(static_cast<uint16_t>(i) + 1, static_cast<uint16_t>(j) + 1)).value() = data[i][j];
    }
}
doc.save()
doc.close()

OpenXLSX::XLDocument doc2;
doc2.create(workbook2, true);
wks = doc2.workbook().worksheet("Sheet1");
// Inserting data into the workbook
for (std::size_t i = 0; i < data.size(); ++i) 
{
    for (std::size_t j = 0; j < data[i].size(); ++j)
    {
        wks.cell(OpenXLSX::XLCellReference(static_cast<uint16_t>(i) + 1, static_cast<uint16_t>(j) + 1)).value() = data[i][j];
    }
}
doc2.save()
doc2.close()

then everything works as expected.
Is the first way not correct?

@aral-matrix
Copy link
Collaborator

Hi @NK01 & thank you for reporting this issue. It sounds like a bug: I see nothing wrong with re-using the XLDocument variable like you do. I will have a look to understand why the document seems to not properly "reset" on ::create, when I find some time - I am currently quite busy & I hope this isn't too urgent, since you have a workaround.

My apologies if this takes a few weeks for me to address.

@NK01
Copy link
Author

NK01 commented Oct 17, 2024

Thank you for the response and yeah it is not urgent, as I have already put the workaround in the code. No worries for the delay, take your time

Repository owner deleted a comment Oct 28, 2024
@aral-matrix aral-matrix self-assigned this Dec 15, 2024
@aral-matrix
Copy link
Collaborator

Thank you for your patience - I just wanted to give you a heads-up: I will start looking into this this week, and might have questions - so if you can check for notifications to respond within a few hours that would be great. :)

@NK01
Copy link
Author

NK01 commented Dec 18, 2024

Thank you for taking up this issue, I am not that well versed in programming to actually contribute to the code however I can help you with any question you might have about the issue

@aral-matrix aral-matrix added the bug Something isn't working label Dec 18, 2024
@aral-matrix
Copy link
Collaborator

Okay, so I was able to reproduce the issue and isolate the root cause: the shared strings cache was not being deleted, so that if you re-assigned the same strings that were already cached to cells in a new document, the XLDocument would think "oh, I already have those strings" and store the indexes from the previous document, but never generate the shared strings table. If you would assign strings not yet cached, they would have been added, but with the wrong index (I didn't verify, the fix was easy enough).

Patched in bf5fe8a - if you could have a look at the development-aral branch, to confirm that this fixes the issue for you as well, that would be appreciated.

Thank you for flagging this, technically speaking this was a memory leak as well (memory from the cache was never released).

@aral-matrix aral-matrix added testing Functionality has been implemented in development branch and is pending a merge into main resolved This issue has been resolved. labels Dec 18, 2024
@SeaonyongPark
Copy link

The below looks similar to this issue.
When opening XLDocument, there is no initialization for list or deque. There might be a problem when opening multiple excel files in application. Would you look at this issue?

void XLDocument::open(const std::string& fileName)
{
// Check if a document is already open. If yes, close it.
if (m_archive.isOpen()) close(); // TBD: consider throwing if a file is already open.
m_filePath = fileName;
m_archive.open(m_filePath);

m_sharedStringCache.clear();   // Added to initialize
m_data.clear();                         // Added to initialize

// ===== Add and open the Relationships and [Content_Types] files for the document level.
std::string relsFilename = "_rels/.rels";
m_data.emplace_back(this, "[Content_Types].xml");
m_data.emplace_back(this, relsFilename);

@NK01
Copy link
Author

NK01 commented Dec 19, 2024

I have tried the patch bf5fe8a - at the development-aral branch, and can confirm that

OpenXLSX::XLDocument doc; // this is used to create and save the workbook
std::vector<std::vector<std::string>> data; // this contains the data to be saved in 2D matrix for easy insertion into excel

doc.create(workbook1, true);
OpenXLSX::XLWorksheet wks = doc.workbook().worksheet("Sheet1");
// Inserting data into the workbook
for (std::size_t i = 0; i < data.size(); ++i) 
{
    for (std::size_t j = 0; j < data[i].size(); ++j)
    {
        wks.cell(OpenXLSX::XLCellReference(static_cast<uint16_t>(i) + 1, static_cast<uint16_t>(j) + 1)).value() = data[i][j];
    }
}
doc.save()
doc.close()

doc.create(workbook2, true);
wks = doc.workbook().worksheet("Sheet1");
// Inserting data into the workbook
for (std::size_t i = 0; i < data.size(); ++i) 
{
    for (std::size_t j = 0; j < data[i].size(); ++j)
    {
        wks.cell(OpenXLSX::XLCellReference(static_cast<uint16_t>(i) + 1, static_cast<uint16_t>(j) + 1)).value() = data[i][j];
    }
}
doc.save()
doc.close()

works without any issues

@aral-matrix
Copy link
Collaborator

aral-matrix commented Dec 19, 2024

@NK01: Thank you for testing :) I will mark this issue as closed when I merge the patch into the main branch!

@aral-matrix
Copy link
Collaborator

@SeaonyongPark:

The below looks similar to this issue. When opening XLDocument, there is no initialization for list or deque. There might be a problem when opening multiple excel files in application. Would you look at this issue?

void XLDocument::open(const std::string& fileName) {
[..]
m_sharedStringCache.clear();   // Added to initialize
m_data.clear();                         // Added to initialize

Thank you for the suggestion, but those are actually initialized with default values in the XLDocument.hpp class definition, line 359 & 360.

        mutable std::list<XLXmlData>    m_data {};              /**<  */
        mutable std::deque<std::string> m_sharedStringCache {}; /**<  */
        mutable XLSharedStrings         m_sharedStrings {};     /**<  */

Clearing them as you suggest is exactly the patch that I added, but in XLDocument::close, which is also invoked by the destructor, if forgotten by the user.

@aral-matrix
Copy link
Collaborator

Okay, this patch is merged into main - closing the issue as resolved.

@aral-matrix aral-matrix removed the testing Functionality has been implemented in development branch and is pending a merge into main label Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working resolved This issue has been resolved.
Projects
None yet
Development

No branches or pull requests

3 participants