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

Reduce provider dependency on internal code #15067

Merged
merged 1 commit into from
Mar 19, 2019
Merged

Conversation

ajcvickers
Copy link
Member

Specifically, refactoring around IRelationalCommand and its interactions so that command building can be overridden without using internals. See issue #11409.

Also, fixes #14923 by taking the logger from SelectExpressionDependencies, which is now scoped.

Made public:

  • RelationalAnnotationsBuilder
  • GetDeclaredForeignKeys
  • GetDeclaredNavigations
  • GetDeclaredProperties (Make sure also for IMutable types...)
  • GetDeclaredServiceProperties
  • GetDeclaredIndexes
  • DisplayName (IEntityType extension)
  • ShortDisplayName (System.Type extension)
  • GetPropertyAccessList
  • RemoveConvert
  • ConfigurationSource
  • CoreAnnotationNames
  • GetAllBaseTypesInclusive
  • Property.Format (made extension method on IEnumerable
  • FindPrincipal
  • ScaffoldingAnnotationNames
  • FindSharedTableRootPrimaryKeyProperty
  • RelationalCommand
  • RelationalCommandBuilderFactory
  • RelationalCommandBuilder

Stopped using in providers:

  • IndentedStringBuilder
  • NonCapturingLazyInitializer

Removed:

  • RelationalParameterBuilder

Also added new parameter objects as needed.

Did not touch the majority of the conventions API. Also, left most query APIs.

Specifically, refactoring around IRelationalCommand and its interactions so that command building can be overridden without using internals. See issue #11409.

Also, fixes #14923 by taking the logger from SelectExpressionDependencies, which is now scoped.

Made public:
* RelationalAnnotationsBuilder
* GetDeclaredForeignKeys
* GetDeclaredNavigations
* GetDeclaredProperties (Make sure also for IMutable types...)
* GetDeclaredServiceProperties
* GetDeclaredIndexes
* DisplayName (IEntityType extension)
* ShortDisplayName (System.Type extension)
* GetPropertyAccessList
* RemoveConvert
* ConfigurationSource
* CoreAnnotationNames
* GetAllBaseTypesInclusive
* Property.Format (made extension method on IEnumerable<IPropertyBase>
* FindPrincipal
* ScaffoldingAnnotationNames
* FindSharedTableRootPrimaryKeyProperty
* RelationalCommand
* RelationalCommandBuilderFactory
* RelationalCommandBuilder

Stopped using in providers:
* IndentedStringBuilder
* NonCapturingLazyInitializer

Removed:
* RelationalParameterBuilder

Also added new parameter objects as needed.

Did not touch the majority of the conventions API. Also, left most query APIs.
@ajcvickers
Copy link
Member Author

@roji @ErikEJ @caleblloyd

image

Beware!

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Cursory review, with my limited knowledge context seems good, I think 🐑 🇮🇹 is the right expression? :)

Ideally I'd be able to build EFCore.PG against this PR to provide more feedback, but I really have no idea where I'd find the nupkgs with the right versions... If you have any knowledge/info let me know. Otherwise we can merge and I can try to sync EFCore.PG soon to raise any issues.

I don't have enough context to understand the change of IRelationalCommandBuilderFactory and ShaperCommandContextFactory from Singleton to Scoped (because of the recent scoped logging), but I'm wondering if the increased scopeness of services impacts perf in any way (i.e. more services need to be created per DbContext)?

@smitpatel
Copy link
Member

Ideally I'd be able to build EFCore.PG against this PR to provide more feedback, but I really have no idea where I'd find the nupkgs with the right versions...

@roji
Copy link
Member

roji commented Mar 19, 2019

@smitpatel I can do that, but is there really no automatic publishing of packages to a nuget feed? This used to be the case with myget, did Arcade remove this?

@smitpatel
Copy link
Member

This is PR build. They were never pushed to myget.

@roji
Copy link
Member

roji commented Mar 19, 2019

My bad, I'll give the packages a try tomorrow.

Copy link
Member

@smitpatel smitpatel left a comment

Choose a reason for hiding this comment

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

:shipit:

@ajcvickers ajcvickers merged commit 049edb0 into master Mar 19, 2019
@ajcvickers
Copy link
Member Author

@roji Singleton to scoped could have a perf impact, but it's cleaner than before. @smitpatel and I talked briefly yesterday about this changing again for the new query pipeline. Let's see how it goes.

@roji
Copy link
Member

roji commented Mar 19, 2019

@ajcvickers ok. Regardless, I'll try to sync EFCore.PG and will report any issues with public/internal.

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

Successfully merging this pull request may close these issues.

Query: SelectExpression holds onto loggers from previous context which compiled the query
3 participants