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.

Sign up for GitHub

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

Make internal SelectExpandBinder methods public #840

Open

bdebaere wants to merge 1 commit into OData:main from bdebaere:expose-selectexpandbinder-methods

bdebaere commented Feb 11, 2023

Because so many methods are marked internal it really makes expanding OData behavior quite a hassle. I implore you to take a look at how extensible EntityFrameworkCore is for example and to reconsider all these internal methods everywhere.


Make internal SelectExpandBinder methods public

d4d3a14

Member

xuzhg commented Feb 13, 2023

Because so many methods are marked internal it really makes expanding OData behavior quite a hassle. I implore you to take a look at how extensible EntityFrameworkCore is for example and to reconsider all these internal methods everywhere.

Can you point me to any EFCore example? Thanks.

Author

bdebaere commented Feb 14, 2023

@xuzhg EntityFrameworkCore has a ServiceCollection where you can add stuff to with for example IDbContextOptionsExtension.ApplyServices(IServiceCollection services). OData has a similar thing where you can add or replace services with ODataOptions.AddRouteComponents(string routePrefix, IEdmModel model, Action<IServiceCollection> configureServices) and this is great!

However, I think you don't take it far enough. Take a look at these EntityFrameworkCore classes/interfaces:

RelationalQueryTranslationPreprocessor
RelationalSqlTranslatingExpressionVisitor
SqlServerQuerySqlGenerator
IMemberTranslatorPlugin
IRelationalTypeMappingSourcePlugin

They allow extending because the necessary methods are public. Too many methods/classes are internal in OData which makes extending it without a custom copy of the code very difficult sometimes.

KenitoInc requested review from KenitoInc, xuzhg, ElizabethOkerio, gathogojr, habbes and lisicase

February 23, 2023 11:53

KenitoInc requested changes

View reviewed changes

src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandBinder.cs

  /// <summary>
  /// For unit test only.
  /// </summary>
- internal SelectExpandBinder()
+ public SelectExpandBinder()

Contributor

KenitoInc Mar 16, 2023

We don't need to make this method public since it's only used internally with some tests.

src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandBinder.cs

  return projectionLambdaExpression;
  }
- internal Expression ProjectAsWrapper(QueryBinderContext context, Expression source, SelectExpandClause selectExpandClause,
+ public Expression ProjectAsWrapper(QueryBinderContext context, Expression source, SelectExpandClause selectExpandClause,

Contributor

KenitoInc Mar 16, 2023

Make method virtual and add docstrings.

src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandBinder.cs

@@ @@ -336,7 +336,7 @@ public class Child : Father @@
  // Generates the expression
  // source => new Wrapper { Instance = source, Container = new PropertyContainer { ..expanded properties.. } }
- internal Expression ProjectElement(QueryBinderContext context, Expression source, SelectExpandClause selectExpandClause, IEdmStructuredType structuredType, IEdmNavigationSource navigationSource)
+ public Expression ProjectElement(QueryBinderContext context, Expression source, SelectExpandClause selectExpandClause, IEdmStructuredType structuredType, IEdmNavigationSource navigationSource)

Contributor

KenitoInc Mar 16, 2023

Make method virtual and add docstrings.

src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandBinder.cs

  /// <param name="propertiesToExpand">The out properties to expand at current level, could be null.</param>
  /// <param name="autoSelectedProperties">The out auto selected properties to include at current level, could be null.</param>
  /// <returns>true if the select contains dynamic property selection, false if it's not.</returns>
- internal static IList<DynamicPathSegment> GetSelectExpandProperties(IEdmModel model, IEdmStructuredType structuredType, IEdmNavigationSource navigationSource,
+ public static IList<DynamicPathSegment> GetSelectExpandProperties(IEdmModel model, IEdmStructuredType structuredType, IEdmNavigationSource navigationSource,

Contributor

KenitoInc Mar 16, 2023

This method is static and cannot be overriden. How do you plan to use it?

src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandBinder.cs

  /// <param name="navigationProperty">The expanded navigation property.</param>
  /// <param name="expandedItem">The expanded navigation select item. It may contain the nested query options.</param>
  /// <param name="includedProperties">The container to hold the created property.</param>
- internal void BuildExpandedProperty(QueryBinderContext context, Expression source, IEdmStructuredType structuredType,
+ public void BuildExpandedProperty(QueryBinderContext context, Expression source, IEdmStructuredType structuredType,

Contributor

KenitoInc Mar 16, 2023

Make method virtual.

src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandBinder.cs

  /// <param name="structuralProperty">The selected structural property.</param>
  /// <param name="pathSelectItem">The selected item. It may contain the nested query options and could be null.</param>
  /// <param name="includedProperties">The container to hold the created property.</param>
- internal void BuildSelectedProperty(QueryBinderContext context, Expression source, IEdmStructuredType structuredType,
+ public void BuildSelectedProperty(QueryBinderContext context, Expression source, IEdmStructuredType structuredType,

Contributor

KenitoInc Mar 16, 2023

Make method virtual.

Contributor

gathogojr commented Mar 29, 2023

@bdebaere Did you get a chance to look at the feedback from @KenitoInc?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment