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

Can we downgrade DocumentFormat.OpenXML to 2.20.0 #14941

Closed
hyzx86 opened this issue Dec 22, 2023 · 31 comments · Fixed by #14989
Closed

Can we downgrade DocumentFormat.OpenXML to 2.20.0 #14941

hyzx86 opened this issue Dec 22, 2023 · 31 comments · Fixed by #14989
Milestone

Comments

@hyzx86
Copy link
Contributor

hyzx86 commented Dec 22, 2023

Can we downgrade DocumentFormat.OpenXML to 2.20.0 ?

I have developed an Excel import/export function that is based on ClosedXML
Unfortunately, it relies on DocumentFormat.OpenXML v2.16.0

While DocumentFormat.OpenXML v3.0.0 was optimized for.NET 8, I tested that all OC unit tests were still available after demoting it to version 2.x

@MikeAlhayek
Copy link
Member

@hyzx86 Enhance your code to use the latest. Don't could on OC to maintain a version that your project is stuck with.

Also, you may be interested in content transfer module which has powerful import/export content items from/excel. It's currently using EPPlus library, but the plan is to remove EPPlus and replace it with another library.

@hyzx86
Copy link
Contributor Author

hyzx86 commented Dec 22, 2023

@MikeAlhayek Yes , I noticed the PR, but there were some licensing issues with EPPlus, so I used ClosedXML, but the DocumentFormat.OpenXML it relies on hasn't been updated yet, Due to some disruptive updates introduced by OpenXML 3.0 to optimize.net8, it is currently cumbersome to update ClosedXML, and since the OC itself does not enforce a dependency on OpenXML 3.0, can it safely downgrade to a lower version?

@MikeAlhayek
Copy link
Member

probably not. Even if we do now, there is nothing that would prevent us from update it again. On the other PR, yes EPPlus will be dropped. Feel free to contribute into that PR by replacing EPPlus with DocumentFormat.OpenXML 3.0

@hyzx86
Copy link
Contributor Author

hyzx86 commented Dec 22, 2023

Yes, I also developed an Excel import/export feature,
But it still has some problems. I think these problems from the OC DefaultContentManager/ContentHandler design problem,
OC will read and write the content repeatedly when updating any content,
Especially CreateAsync, UpdateAsync, PublishAsync
The three most common operations above all repeatedly read and write to the database, resulting in a very slow import process

Will this issue be optimized in this PR?

@hyzx86
Copy link
Contributor Author

hyzx86 commented Dec 22, 2023

probably not. Even if we do now, there is nothing that would prevent us from update it again. On the other PR, yes EPPlus will be dropped. Feel free to contribute into that PR by replacing EPPlus with DocumentFormat.OpenXML 3.0

which one?

@MikeAlhayek
Copy link
Member

content transfer module

@hyzx86
Copy link
Contributor Author

hyzx86 commented Dec 22, 2023

Oh, haha, that's what I'm looking at. I didn't know there was another one

@hyzx86
Copy link
Contributor Author

hyzx86 commented Dec 22, 2023

Because my project structure is front-end separated and not included in the OC's module, I created a ScirptQuery that uses a Jint script to configure how to import/export the content
Just like this:

//read excel from upload
const excelData = readExcel({
    sheetName: "Trading",
    startAddress: "A3"
})

//  const x ={
//                 "SOA": "test",
//                 "Incoterm": "DDP",
//                 "Source_Country": "CH",
//                 "Destination_Country": "IN",
//                 "Total_Freight_": 5.0,
//                 "Total_Duty_": 5.0,
//                 "Other_Charges_": 5.0
//             }


const contentItems = excelData.map(x => {
    const content = newContentProperties({
        TestTrading: {
            SOA:{Text:x.SOA},
            Incoterm:{Text:x.Incoterm},
            SourceCountry:{Text:x.Source_Country},
            DestinationCountry:{Text:x.Destination_Country},
            TotalFreight:{Text:x.Total_Freight_},
            TotalDuty:{Text:x.Total_Duty_},
            OtherCharges:{Text:x.Other_Charges_},
        }
    })
    return content;
})
clearContentByContentType(types.IndexTypes.TestTrading)
const total = importContentItems({
    contentModelList: contentItems,
    contentType: types.IndexTypes.TestTrading,
    useGraphqlFormat:false,
    useDefaultImport:false,
    publish: true
})


return {
    data: {
        total
    }
}

@sebastienros
Copy link
Member

We discussed about moving Indexing to its own module to separate it from Media, and then extending this to have a module for Pdf indexing and Word indexing too as they are adding an external dependency that we might not want to spread with each deployment.

@sebastienros
Copy link
Member

sebastienros commented Jan 4, 2024

It looks like the feature Media.Indexing doesn't have any external dependency at all and could stay in the Media module. But each of Pdf and MicrosoftOffice indexing should be in their own module (one for each).

@hishamco
Copy link
Member

hishamco commented Jan 4, 2024

Creating a module for each one will be too much, I presume the OrchardCore.Media.Indexing has a dependency to DocumentFormat.OpenXML` is it?

@sebastienros
Copy link
Member

If a feature requires a nuget package it should introduce a module to prevent the issue that is described here.

@MikeAlhayek
Copy link
Member

@sebastienros are you saying here we should add 2 new modules of for PDF and other for others? I think this would be an over kill. I create a PR that move all indexing in a separate module to but split them into multiple features. I think one module for all indexing is enough unless someone else complains about PdfPig project

@sebastienros
Copy link
Member

Compromise ...

@MikeAlhayek
Copy link
Member

@sebastienros does that means you are okay for 1 additional project or you still prefer 2 as described in my last comment?

@Skrypt
Copy link
Contributor

Skrypt commented Jan 5, 2024

One module per external dependency required would be better as suggested by @sebastienros
Else if you bundle them up all in a single module you will end up with the same issue about people complaining that they don't want to upgrade a specific one.

As long as they are named something like OrchardCore.Media.Indexing.Pdf ... OrchardCore.Media.Indexing.MSDoc

@mariojsnunes
Copy link
Contributor

Can we downgrade DocumentFormat.OpenXML to 2.20.0 ?

I have developed an Excel import/export function that is based on ClosedXML Unfortunately, it relies on DocumentFormat.OpenXML v2.16.0

I'm also facing the same issue, but it seems they are upgrading it soon, so this might not be necessary.
ClosedXML/ClosedXML#2220 (comment)

@hyzx86
Copy link
Contributor Author

hyzx86 commented Jan 6, 2024

link to : ClosedXML/ClosedXML#2248

@mariojsnunes
Copy link
Contributor

The dependency on DocumentFormat.OpenXML is still v3.0.0.
Are we waiting for 3.0.1?
Asking because this was closed but the problem isn't solved yet.

I have developed an Excel import/export function that is based on ClosedXML Unfortunately, it relies on DocumentFormat.OpenXML v2.16.0

@hyzx86
Copy link
Contributor Author

hyzx86 commented Jan 11, 2024

It seems that people are not ready for 3.0, because upgrading 3.0 will introduce many destructive updates.
Fortunately, however, these updates do not seem to affect the existing functions of OC.

dotnet/Open-XML-SDK#1636
dotnet/Open-XML-SDK#1637

@hishamco
Copy link
Member

@MikeAlhayek is your PR fixing the issue?

@MikeAlhayek
Copy link
Member

If you use latest preview 1.9, you'll notice that the only package we use OpenXML is in OrchardCore.Indexing.OpenXML. You should be able to not reference it in your project and maybe able to override the version in your own project. Can you try it out?

@hyzx86
Copy link
Contributor Author

hyzx86 commented Jan 11, 2024

If you use latest preview 1.9, you'll notice that the only package we use OpenXML is in OrchardCore.Indexing.OpenXML. You should be able to not reference it in your project and maybe able to override the version in your own project. Can you try it out?

Ok, I'll try, but if I want to use OrchardCore.Indexing.OpenXML, so I will need to copy the source code to my project and then to downgrade its reference?

@MikeAlhayek
Copy link
Member

yes.

@hyzx86
Copy link
Contributor Author

hyzx86 commented Jan 11, 2024

Um, well, MediaFile Indexing is not as important to me as Excel import

I've actually copy all of the media module in my project 🤣

Because the problem is hidden, it can only be discovered at actual runtime, but it has been updated several versions by the time it is discovered

@hishamco
Copy link
Member

Can you try it out?

@hyzx86 could you please try it while you filed the issue, I'm currently busy with other PRs

@mariojsnunes
Copy link
Contributor

I don't have OrchardCore.Indexing.OpenXML referenced anywhere, and still get the original issue. (Using 1.8.1)

maybe able to override the version in your own project

Unsure how could do this. But if it involves copying/overriding source code would be an ugly solution I'd like to avoid.

@hyzx86
Copy link
Contributor Author

hyzx86 commented Jan 12, 2024

I don't have OrchardCore.Indexing.OpenXML referenced anywhere, and still get the original issue. (Using 1.8.1)

I remember upgrading OpenXML was a change in the last few months, maybe you can try to downgrade to v1.7.1 of OC

OrchardCore.Indexing.OpenXML module's purpose is to break up,
But this PR was recently submitted by Mike, so it's not included in the release yet, and you'll need to upgrade to the latest preview

If you can't downgrade OC, you can try upgrading to the preview version
And then to achieve a similar OrchardCore.Application.Cms.Core.Targets project, according to the need to adjust your references, and remove OrchardCore.Indexing.OpenXML

@hyzx86
Copy link
Contributor Author

hyzx86 commented Jan 14, 2024

Can you try it out?

@hyzx86 could you please try it while you filed the issue, I'm currently busy with other PRs

@hishamco I'm waiting for this PR to merge into the main branch

#15003

@hishamco
Copy link
Member

What about the current issue?

@hyzx86
Copy link
Contributor Author

hyzx86 commented Jan 15, 2024

What about the current issue?

I'm all right

@MikeAlhayek MikeAlhayek modified the milestones: 2.x, 2.0 Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants