dotnetcore / NPOI

A .NET library for reading and writing Microsoft Office binary and OOXML file formats.
Apache License 2.0
1.93k stars 413 forks source link

Border style unintentionally shared between XSSFStyle instances #157

Open vpstuart opened 4 years ago

vpstuart commented 4 years ago

I have found in an application that the border formatting in XSSFCellStyle is a bit broken. It results in accidentally sharing a CT_Border instance.

From https://github.com/dotnetcore/NPOI/blob/master/src/NPOI.OOXML/XSSF/UserModel/XSSFCellStyle.cs#L227

        public BorderStyle BorderBottom
        {
            get
            {
                if (!_cellXf.applyBorder) return BorderStyle.None;

                int idx = (int)_cellXf.borderId;
                CT_Border ct = _stylesSource.GetBorderAt(idx).GetCTBorder();
                if (!ct.IsSetBottom())
                {
                    return BorderStyle.None;
                }
                else
                {
                    return (BorderStyle)ct.bottom.style;
                }
            }
            set
            {
                CT_Border ct = GetCTBorder();
                CT_BorderPr pr = ct.IsSetBottom() ? ct.bottom : ct.AddNewBottom();
                if (value == BorderStyle.None) 
                    ct.UnsetBottom();
                else 
                    pr.style = (ST_BorderStyle)value;

                int idx = _stylesSource.PutBorder(new XSSFCellBorder(ct, _theme));

                _cellXf.borderId = (uint)idx;
                _cellXf.applyBorder = (true);
            }
        }

Each of the border setters has the same but, I have chosen BorderBottom as an example.

The first time you assign a value to a border property. E.g. cell.BorderBottom = BorderStyle.Thin; it will create a CT_Border object, assign the border to it and then call _stylesSource.PutBorder(new XSSFCellBorder(ct, _theme)) which will store the border object and return an ID.

If it finds a matching CT_Border it will return that instances id, causing it to be shared.

This seems to be from here: https://github.com/dotnetcore/NPOI/blob/master/src/NPOI.OOXML/XSSF/Model/StylesTable.cs#L285

        public int PutBorder(XSSFCellBorder border)
        {
            int idx = borders.IndexOf(border);
            if (idx != -1)
            {
                return idx;
            }
            borders.Add(border);
            border.SetThemesTable(theme);
            return borders.Count - 1;
        }

Using borders.IndexOf(border) will search by the current value of the border. The XSSFCellBorder class implements equality based on the XML value of the CT_Border.

I think the easiest and possibly best solution would be to delete the 5 lines:

            int idx = borders.IndexOf(border);
            if (idx != -1)
            {
                return idx;
            }

I'm not a regular contributer here, but if it would help I can try and put together a PR.

Warm Regards, Stuart

vpstuart commented 4 years ago

On second look that will result in the pool of borders growing each time. Perhaps instead CT_Border ct = GetCTBorder(); needs to always make a clone.

gbs56 commented 4 years ago

I didn't know the cause (what you describe) but this was happening to me. Define a border in one style affects any other style that has border definition. Good finding. Thanks, Gustavo