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

Move CosmosConventionSetBuilder out of internal apis. #28220

Closed
AraHaan opened this issue Jun 13, 2022 · 7 comments
Closed

Move CosmosConventionSetBuilder out of internal apis. #28220

AraHaan opened this issue Jun 13, 2022 · 7 comments
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@AraHaan
Copy link
Member

AraHaan commented Jun 13, 2022

When one wants to extend the default CosmosConventionSetBuilder to add additional conventions (for non-relational dbs) they would need to extend the original CosmosConventionSetBuilder type which generates compile warnings due to it being marked by it as an internal API.

I would like to propose for it to be allowed to extend it's conventions like the other 3 providers in the repository that are shipped in it's 6.0.x nuget packages.

I think the best solution is for the API to be moved out of internal so that way it can be extended without generating any compile warnings. Also I do not see it's ConventionSetBuilder getting deleted anytime soon in the future so I think it's type is safe to move out of internal anyway.

@AraHaan
Copy link
Member Author

AraHaan commented Jun 13, 2022

Example extension to the CosmosConvensionSetBuilder that one might create for their application (or helper library for it):

namespace Microsoft.EntityFrameworkCore.Metadata.Conventions;

using Microsoft.EntityFrameworkCore.Cosmos.Metadata.Conventions.Internal;

public class CosmosExtendedConventionSetBuilder : CosmosConventionSetBuilder
{
    public CosmosExtendedConventionSetBuilder(
        ProviderConventionSetBuilderDependencies dependencies)
        : base(dependencies)
    {
    }

    /// <summary>
    ///     Builds and returns the convention set for the current database provider.
    /// </summary>
    /// <returns>The convention set for the current database provider.</returns>
    public override ConventionSet CreateConventionSet()
    {
        var conventionSet = base.CreateConventionSet();
        conventionSet.AddExtendedConventionSet();
        return conventionSet;
    }
}

Where AddExtendedConventionSet is an extension to ConventionSet that they also define that then could be shared with multiple extended providers.

@ajcvickers
Copy link
Member

@AraHaan Isn't this part of #214?

@AraHaan
Copy link
Member Author

AraHaan commented Jun 13, 2022

I did not see the CosmosConventionSetBuilder listed explicitly on there, but it can be added to that list for that issue too.

I think an alternative option is to migrate all of the code in the current providers that override CreateConventionSet into extension methods of ConventionSet that I do (which would actually help others create Relational or non-relational conventions on top of ones normally used in predefined providers without deriving from their class).

@roji
Copy link
Member

roji commented Jun 13, 2022

@AraHaan RelationalConventionSetBuilder and ProviderConventionSetBuilder, which contain conventions meant for use by concrete providers, are already public. I'm not sure why someone would need to access CosmosConventionSetBuilder - the conventions there are Cosmos-specific, so they're not meant to be usable by anyone except if they're maybe creating a tweaked Cosmos provider based on the official one... If the idea is simply to add arbitrary conventions, that can already be done via a plugin.

What is the concrete use case you need this for?

@AraHaan
Copy link
Member Author

AraHaan commented Jun 13, 2022

I am not sure what providers all use IConventionSetPlugin (or if any of them directly use it) as such I am not sure if it would work for all of them out of box.

Likewise, what if I have an assembly that provides both convensions for non-relational and relational databases, how could there be an Relational only version of IConventionSetPlugin?

I think this is why I extend the providers for now until I have more information on the plugins or if they would be perfect for my exact usage.

The plugins seem to have worked, however I do not like the drawback of still needing to add my own IServiceCollection extension for adding each plugin (depending on if the user is using an relational or non-relational db).

@roji
Copy link
Member

roji commented Jun 13, 2022

The plugins seem to have worked, however I do not like the drawback of still needing to add my own IServiceCollection extension for adding each plugin (depending on if the user is using an relational or non-relational db).

I'm not sure why the relational/non-relational difference is relevant here - a plugin simply adds an IConventionSetPlugin which gets picked up and used.

If you're referring to the need to have an IServiceCollection at all, and are simply looking for an easy way to add a convention in a specific application (without it being reusable across different projects), then that's what #214 is about. But there always must be some gesture to actually perform that - register the plugin, or register the user convention.

In any case, this really is quite abstract, if you have further questions then a concrete scenario would be helpful.

@ajcvickers
Copy link
Member

Discussed in triage and decided there is nothing to do here that is not covered by #214.

@ajcvickers ajcvickers added closed-no-further-action The issue is closed and no further action is planned. and removed type-enhancement labels Jun 20, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

3 participants