jmcnamara / rust_xlsxwriter

A Rust library for creating Excel XLSX files.
https://crates.io/crates/rust_xlsxwriter
Apache License 2.0
316 stars 25 forks source link

Possible bug with sheet name apostrophes in ChartRange #44

Closed 2ndDerivative closed 1 year ago

2ndDerivative commented 1 year ago

Current behavior

"'" is only stripped from sheet name in ChartRange if both start and beginning are apostrophes

Expected behavior

Either ' in the beginning or the end of the sheet name in a Chart range would be stripped, since the excel standard requires them to be removed independently

Sample code to reproduce

use rust_xlsxwriter::{Workbook, XlsxError};

fn main() -> Result<(), XlsxError> {
    // This apostrophe would not be stripped since the code only strips if both end and beginning
    // of the chart name are apostrophes
    let chart = ChartRange::new_from_string("\'SheetName!A1:B2");
}

Environment

- rust_xlsxwriter version:
- rustc version:
- Excel version:
- OS:

Any other information

No response

jmcnamara commented 1 year ago

Thanks for the detailed report.

As reported this isn't a bug since it isn't intended, in rust_xlsxwriter that the apostrophes are stripped in general. They are only removed when they are "notional", i.e., when they indicate the sheet name contains whitespace.

There probably does need to be centralised name checking, like you suggested in #45. I'll look into that.

I also need to check if there is a bug if an invalid worksheet name is passed as a chart range. I'll keep this open and look into that.

Thanks again.

2ndDerivative commented 1 year ago

According to the Microsoft spec it's not supposed to use apostrophes in general if I read this right https://support.microsoft.com/en-us/office/rename-a-worksheet-3f1f7148-ee83-404d-8ef0-9ff99fbad1f9

jmcnamara commented 1 year ago

According to the Microsoft spec it's not supposed to use apostrophes in general if I read this right

Correct but rust_xlsxwriter doesn't sanitize the input (in this case). This would be considered a user input error and should raise a validation error.

However, I just checked and it doesn't:

use rust_xlsxwriter::{Chart, ChartType, Workbook, XlsxError};

fn main() -> Result<(), XlsxError> {
    let mut workbook = Workbook::new();
    let worksheet = workbook.add_worksheet();

    let data = [1, 2, 3, 2, 1];
    worksheet.write_column(0, 0, data)?;

    let mut chart = Chart::new(ChartType::Column);
    chart.add_series().set_values("'Sheet1!$A$1:$A$5");
    worksheet.insert_chart(0, 2, &chart)?;

    workbook.save("chart.xlsx")?;

    Ok(())
}
$ cargo run --example gh44    # No error.

So I'll look into that and fix it so that there is proper validation.

2ndDerivative commented 1 year ago

That makes sense. So it would get plugged into the central system I suggested, that would be more idiomatic.

jmcnamara commented 1 year ago

Closing this and merging the issue with #45