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

Remove Newtonsoft #14572

Merged
merged 117 commits into from
Feb 2, 2024
Merged

Remove Newtonsoft #14572

merged 117 commits into from
Feb 2, 2024

Conversation

jtkech
Copy link
Member

@jtkech jtkech commented Oct 24, 2023

The biggest breaking change I never did ;)

For now more like a learning branch not ready to be reviewed.

Nothing works but at least the web app builds (not yet the tests).

Fixes #14456.

@hishamco
Copy link
Member

hishamco commented Oct 24, 2023

@jtkech me & @DAud-IcI planned to migrate to STJ #14456

Personally, I made some progress in the PR #14457, there is a local changes that have not pushed yet, so we don't duplicate the work or waste time for both of us unless you are working on something related :)

@sarahelsaig
Copy link
Contributor

@jtkech me & @DAud-IcI planned to migrate to STJ #14456

Personally, I made some progress in the PR #14457, there is a local changes that have not pushed yet, so we don't duplicate the work or waste time for both of us unless you are working on something related :)

Yes, I too have made some progress building on @hishamco 's PR in this in the last few weeks. Just forgot to make a draft PR till now, but here it is #14609

@hishamco
Copy link
Member

I also made some progress, but seems JT keeps committing without checking our comments :)

@jtkech
Copy link
Member Author

jtkech commented Oct 30, 2023

No problem, as mentioned this branch was more like a learning branch, but for now I have no time to continue, I will when I will have time.

That said the full Blog setup passes, and quite all unit tests unless some related to GraphQl and those related to Deployment that is not yet converted to SJT.

The main remaining thing is how to make JsonObject working as dynamic as it is used for example with Shapes and GraphQl arguments, but maybe better to remove all our usage of dynamic things.

@sebastienros
Copy link
Member

@jtkech Look at my branch in yessql for how to get it to work with dynamics. There is a special converter based on the returned type.

@jtkech
Copy link
Member Author

jtkech commented Oct 31, 2023

@sebastienros Thanks, yes I already tried it but even if it returns an ExpandoObject it was not fixing the GraphQl arguments, I will retry asap but this night I need to focus on this issue #14574.

@jtkech
Copy link
Member Author

jtkech commented Oct 31, 2023

@sebastienros

Yes the dynamic converter works, It needed to be added somewhere else.

@MikeAlhayek
Copy link
Member

@sebastienros there is one more issue.

has occurred while executing the request. System.ArgumentNullException: Value cannot be null. (Parameter 'key')
   at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
   at YesSql.Indexes.IdentityMap.AddEntity(Int64 id, Object entity)
   at YesSql.Session.Get[T](IList`1 documents, String collection)
   at YesSql.Services.DefaultQuery.Query`1.FirstOrDefaultImpl()
   at YesSql.Services.DefaultQuery.Query`1.FirstOrDefaultImpl()
   at OrchardCore.Data.Documents.DocumentStore.GetOrCreateImmutableAsync[T](Func`1 factoryAsync) in C:\Code\OrchardCMS\OrchardCore\src\OrchardCore\OrchardCore.Data.YesSql\Documents\DocumentStore.cs:line 56
   at OrchardCore.Documents.DocumentManager`1.GetOrCreateImmutableAsync(Func`1 factoryAsync) in C:\Code\OrchardCMS\OrchardCore\src\OrchardCore\OrchardCore.Infrastructure\Documents\DocumentManager.cs:line 143
   at OrchardCore.Layers.Services.LayerFilter.OnResultExecutionAsync(ResultExecutingContext context, ResultExecutionDelegate next) in C:\Code\OrchardCMS\OrchardCore\src\OrchardCore.Modules\OrchardCore.Layers\Services\LayerFilter.cs:line 91
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResultFilterAsync>g__Awaited|30_0[TFilter,TFilterAsync](ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)

@MikeAlhayek
Copy link
Member

@sebastienros the last error is strange. After digging little more here is the error that I am getting

Deserialization of types without a parameterless constructor, a singular parameterized constructor, or a parameterized constructor annotated with 'JsonConstructorAttribute' is not supported. Type 'OrchardCore.Rules.Models.StringOperator'. Path: $.Layers[2].LayerRule.Conditions[0].Operation | LineNumber: 0 | BytePositionInLine: 669.'

the StringOperator has parameter-less constructor. and $.Layers[2].LayerRule.Conditions[0].Operation

Here is the layer JSON from the database

{
    "Layers":[
        {
            "Name":"Knowledge Base",
            "LayerRule":{
                "Conditions":[
                    {
                        "$type":"OrchardCore.Rules.Models.AnyConditionGroup, OrchardCore.Rules",
                        "Conditions":[
                            {
                                "$type":"OrchardCore.Rules.Models.UrlCondition, OrchardCore.Rules",
                                "Value":"/knowledge-base/category/",
                                "Operation":{
                                    "$type":"OrchardCore.Rules.Models.StringStartsWithOperator, OrchardCore.Rules",
                                    "CaseSensitive":false
                                },
                                "Name":"UrlCondition",
                                "ConditionId":"4q7ayd91z69gzv8qzta07rtpm0"
                            },
                            {
                                "$type":"OrchardCore.Rules.Models.ContentTypeCondition, OrchardCore.Rules",
                                "Value":"KnowledgeBaseArticle",
                                "Operation":{
                                    "$type":"OrchardCore.Rules.Models.StringEqualsOperator, OrchardCore.Rules",
                                    "CaseSensitive":false
                                },
                                "Name":"ContentTypeCondition",
                                "ConditionId":"4bee24643tz0nsgwn948venadz"
                            },
                            {
                                "$type":"OrchardCore.Rules.Models.RoleCondition, OrchardCore.Rules",
                                "Value":"/search",
                                "Operation":{
                                    "$type":"OrchardCore.Rules.Models.StringStartsWithOperator, OrchardCore.Rules",
                                    "CaseSensitive":false
                                },
                                "Name":"RoleCondition",
                                "ConditionId":"4017ez2rhw7za5fxmcn0kgbw5k"
                            }
                        ],
                        "Name":"AnyConditionGroup",
                        "ConditionId":"49kzd1py7h3vnx29608cmmta5a"
                    }
                ],
                "ConditionId":"4jm7m68aj30g0rczdne1c4b1cp"
            }
        },
        {
            "Name":"Knowledge Base Documents",
            "LayerRule":{
                "Conditions":[
                    {
                        "$type":"OrchardCore.Rules.Models.AnyConditionGroup, OrchardCore.Rules",
                        "Conditions":[
                            {
                                "$type":"OrchardCore.Rules.Models.UrlCondition, OrchardCore.Rules",
                                "Value":"/knowledge-base/category/",
                                "Operation":{
                                    "$type":"OrchardCore.Rules.Models.StringStartsWithOperator, OrchardCore.Rules",
                                    "CaseSensitive":false
                                },
                                "Name":"UrlCondition",
                                "ConditionId":"4rgsspd31y3m67jk68q0ny57ng"
                            },
                            {
                                "$type":"OrchardCore.Rules.Models.ContentTypeCondition, OrchardCore.Rules",
                                "Value":"KnowledgeBaseArticle",
                                "Operation":{
                                    "$type":"OrchardCore.Rules.Models.StringEqualsOperator, OrchardCore.Rules",
                                    "CaseSensitive":false
                                },
                                "Name":"ContentTypeCondition",
                                "ConditionId":"4s8zc1yn4qg64xscsdth3c6jcf"
                            }
                        ],
                        "Name":"AnyConditionGroup",
                        "ConditionId":"4jd36bg0n6p2htsvrw5vgf6cdt"
                    }
                ],
                "ConditionId":"4ywdwwjm06x2t2t0yr332en2wd"
            }
        },
        {
            "Name":"Home Page",
            "Description":"A layer to make content visible on home page.",
            "LayerRule":{
                "Conditions":[
                    {
                        "$type":"OrchardCore.Rules.Models.HomepageCondition, OrchardCore.Rules",
                        "Value":true,
                        "Name":"HomepageCondition",
                        "ConditionId":"4aqqr1zz96va9s6b3nhy9afvw6"
                    }
                ],
                "ConditionId":"4kpbhnws6dm14yxyc0rt4phw5p"
            }
        },
        {
            "Name":"Secure Home Page",
            "Description":"A layer to make content visible on home page when the user is authenticated.",
            "LayerRule":{
                "Conditions":[
                    {
                        "$type":"OrchardCore.Rules.Models.IsAuthenticatedCondition, OrchardCore.Rules",
                        "Name":"IsAuthenticatedCondition",
                        "ConditionId":"4cw5t50rfyvm61agcs1xs2v6x6"
                    },
                    {
                        "$type":"OrchardCore.Rules.Models.HomepageCondition, OrchardCore.Rules",
                        "Value":true,
                        "Name":"HomepageCondition",
                        "ConditionId":"4jm3g93fvqg3k6ese610vtvzcf"
                    }
                ],
                "ConditionId":"4jvy81v5dvem66dk43ysqr64xv"
            }
        },
        {
            "Name":"Anonymous Home Page",
            "Description":"A widget layer for unauthenticated users.",
            "LayerRule":{
                "Conditions":[
                    {
                        "$type":"OrchardCore.Rules.Models.HomepageCondition, OrchardCore.Rules",
                        "Value":true,
                        "Name":"HomepageCondition",
                        "ConditionId":"4b14hdp7q60xw63ybp0q2wedxt"
                    },
                    {
                        "$type":"OrchardCore.Rules.Models.IsAnonymousCondition, OrchardCore.Rules",
                        "Name":"IsAnonymousCondition",
                        "ConditionId":"4tcmmkf7mzsjd41awsgf3bjy5s"
                    }
                ],
                "ConditionId":"4trw79793xqg4r1hkdfv6s83dj"
            }
        }
    ],
    "Identifier":"4cy4fpeft8xsywx7f1v6w2bjdt"
}

@sebastienros
Copy link
Member

The Rules issue is because there is a hierarchy to annotate. I am fixing it, using a different way than the services though.

@MikeAlhayek
Copy link
Member

@sebastienros thank you! that fixed the last issues I was able to spot. Can you please look at the release notes to make sure I did not miss anything that needs to be done in order to upgrade?

https://github.com/OrchardCMS/OrchardCore/blob/b3a9ab4ae5b07d06a5eb1729dd0ef1fd0d633a3f/src/docs/releases/1.9.0.md

Copy link
Member

@MikeAlhayek MikeAlhayek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jtkech! You are still contributing my friend!

@sebastienros
Copy link
Member

I have changed it to use options instead of the DI. It still worked on my machine after (tried deployments and layer rules). The goal was to have less things in DI.

@sebastienros
Copy link
Member

I started a functional tests run on this branch before we merge.

@sebastienros
Copy link
Member

Updated release notes, functional tests passed.

@MikeAlhayek MikeAlhayek merged commit 71fde7c into main Feb 2, 2024
5 checks passed
@MikeAlhayek MikeAlhayek deleted the jtkech/remove-newton branch February 2, 2024 19:22
@MikeAlhayek
Copy link
Member

@Piedone This beast was just merged. Can you please use dotnest or other ways to test it out to make sure we did not break anything?

cc: @hishamco @Skrypt @agriffard

@hishamco
Copy link
Member

hishamco commented Feb 2, 2024

Do we have a release note, because this is HUGE!!

@MikeAlhayek
Copy link
Member

yes it should be part of the 1.9 release notes. I think it should be published soon. guessing it take a while for the docs to be published

@Piedone
Copy link
Member

Piedone commented Feb 4, 2024

We'll first need to finish the 1.8 upgrade but after that we'll look into this: Lombiq/Open-Source-Orchard-Core-Extensions#692. Please wait with an 1.9 release for that because that can bring up a lot of feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate from Newtonsoft.Json to System.Text.Json
7 participants