OData / odata.net

ODataLib: Open Data Protocol - .NET Libraries and Frameworks
https://docs.microsoft.com/odata
Other
675 stars 348 forks source link

Support net8.0 - Avoid evaluating Current on an empty enumerator #2974

Closed lewing closed 1 month ago

lewing commented 1 month ago

Issues

This pull request fixes #2796

Description

Avoid calling .Current on an empty enumerator. Calling Current is not valid unless MoveNext() has returned true and in net8.0 this was enforced.

Checklist (Uncheck if it is not completed)

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

lewing commented 1 month ago

I went with the simplest possible fix here at first but decided to make it more performant and readable (to me at least) by avoiding iterating at all.

        public ODataPathInfo(ODataPath odataPath)
        {
            ODataPathSegment targetSegment = odataPath.LastSegment;

            if (targetSegment != null)
            {
                // use next to last segment if the last one is Key or Count Segment
                if ((targetSegment is KeySegment || targetSegment is CountSegment) && odataPath.Count > 1)
                {
                    targetSegment = odataPath[odataPath.Count - 2];
                }

                this.targetNavigationSource = targetSegment.TargetEdmNavigationSource;
                this.targetEdmType = targetSegment.EdmType;
                if (this.targetEdmType != null)
                {
                    IEdmCollectionType collectionType = this.targetEdmType as IEdmCollectionType;
                    if (collectionType != null)
                    {
                        this.targetEdmType = collectionType.ElementType.Definition;
                    }
                }
            }

            this.segments = odataPath;
        }
richlander commented 1 month ago

FYI: I was at the .NET Booth at Build today. A customer told me that the associated bug is their primary/only blocker from moving to .NET 8. It would be great to see this fix merged.

Thanks in advance.

Stephanvs commented 1 month ago

FYI: I was at the .NET Booth at Build today. A customer told me that the associated bug is their primary/only blocker from moving to .NET 8. It would be great to see this fix merged.

Thanks in advance.

Same thing is true in my case as well. Hope to see this fixed soon! 👍

lewing commented 1 month ago

cc @ElizabethOkerio (I can't request reviewers)

habbes commented 1 month ago

@lewing good call to avoid enumeration. That code must have been written before we added an indexer to ODataPath.

habbes commented 1 month ago

FYI: I was at the .NET Booth at Build today. A customer told me that the associated bug is their primary/only blocker from moving to .NET 8. It would be great to see this fix merged.

Thanks in advance.

@richlander is that customer an OData user?

richlander commented 1 month ago

@habbes -- yes. They told me use the odata package on nuget.org. I have not seen them again at the booth.

richlander commented 1 month ago

Looks like the commit got reverted @mikepizzo? Were there test failues?

mikepizzo commented 1 month ago

@gathogojr wanted to add some specific tests for the (previously) failing case. He will add the tests and do a build tonight.

mikepizzo commented 1 month ago

I restored the previous commit as John has added his tests to a separate PR.