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

fix binding readonly properties #1031

Merged

Conversation

ElizabethOkerio
Copy link
Contributor

@ElizabethOkerio ElizabethOkerio commented Aug 23, 2023

This PR fixes #1026

In this fix #993 we added a fix to remove duplicate joins from the generated sql. This involved making updates to the Instance property of the expression to be created and only including structural properties.

The changes led to this being generated:

{ Instance = new Customer() {Id = $it.Id, Name= $it.Name}}

Instead of this:

{ Instance = $it }
    foreach (PropertyInfo prop in props)
            {
                foreach (var sp in structuralProperties)
                {
                    if (sp.Name == prop.Name)
                    {
                        MemberExpression propertyExpression = Expression.Property(source, prop);
                        bindings.Add(Expression.Bind(prop, propertyExpression));
                        break;
                    }
                }
            }

This is the code that recreates the Instance to only have structural properties.. Calling Bind on a read-only property that doesn't have a setter will throw exceptions. This fix adds a check to check if a property has a setter. If it has a setter then we can do a bind, otherwise we don't.

The readonly properties will be on the response. This won't affect.

xuzhg
xuzhg previously approved these changes Aug 25, 2023
@gathogojr
Copy link
Contributor

@ElizabethOkerio When we had, { Instance = $it }, what was preventing the readonly properties from causing chaos given that $it implies everything (I think)?

Copy link
Contributor

@habbes habbes left a comment

Choose a reason for hiding this comment

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

Would be great to explain in the PR description why read-only properties are included in the response even when they are not explicitly added to the instance.

@ElizabethOkerio ElizabethOkerio merged commit 2bde4e5 into OData:main Aug 28, 2023
2 checks passed
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.

Regression: 8.2.1 Does throws exception on read-only properties on relationship entities on expansion
4 participants