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

Add bound flag and make namespace consistent #448

Merged
merged 2 commits into from Jul 5, 2016

Conversation

Projects
None yet
4 participants
@chinadragon0515
Contributor

chinadragon0515 commented Jun 20, 2016

Issues

*This pull request fixes issue #443 and #438 *

Description

Will make default namespace consistent and also add a new method for new entity type extend which will be used for later operation build.

Checklist (Uncheck if it is not completed)

  • [ x ] Test cases added
  • [ x ] Build and test with one-click build and test script passed

Additional work necessary

Documents is needed for new model extend.

/// A sub class need to be implemented and register as DI service like "AddTransient<RestierModelBuilder, CustomizedRestierModelBuilder>"
/// </summary>
/// <param name="builder"></param>
public virtual void ExtendModel(ODataConventionModelBuilder builder)

This comment has been minimized.

@rayao

rayao Jun 20, 2016

Contributor

As we have DI now, such virtual methods could be abstracted into an interface, like IModelExtender, so that consumer won't have to inherit RestierModelBuilder. And that way even chained model extenders are automatically supported.

This comment has been minimized.

@chinadragon0515

chinadragon0515 Jun 21, 2016

Contributor

@rayao we already has model builder as chain service, but it does not support a case very well when we make EF entity type and set model build in OData publisher which is user need to add more entity type and set before operation model builder starts, I assume this is for very small group of consumers but we do see consumers need this now.
We have several options for consumers,

  1. Without this method, user can still do this, but user need to overwrite all logic in RestierModelExtender and add his own logic
  2. Add a new virtual method to allow user add new entity set and entity type before operation model build start.
  3. Introduce a new interface and another chain of service as you proposed.
    I was thinking the third option is too complex and even normal user will confuse with IModelBuilder and IModelExtender.

We will re-think this when we resolve issue #447, we may need to move operation model builder from ODL to Web Api model builder too.

This comment has been minimized.

@robertmclaws

robertmclaws Jun 21, 2016

Collaborator

So, I think you guys are making this entirely too complicated, partially because you are not implementing DI correctly. The point is to allow the end-user to manipulate the model that gets used by RESTier. Nearly 100% of users will be used to using the ODataConventionModelBuilder, because that is what is used by WebAPI. I think you would be hard-pressed to find anyone who wants to manipulate the result of the builder.GetEdmModel() call. If that is the case, than this function is returning entirely the wrong object type.

Further, the point of Dependency Injection is to create base classes that can be inherited from and used, not to create chains that have to be implemented in specific orders... that can be done without DI. All of the "chain of service" stuff is overcomplicating everything about how the end-developer interacts with the service. It's totally counter-intuitive. I know this because I spent 12 hours over the weekend trying to implement some of this stuff in an API, and it is very confusing.

I personally think the design should look like this:

///<summary>
/// Handles the automagic process of constructing a ModelBuilder instance from a given DbContext.
///</summary>
///<remarks>
/// Inherit from this class, override BuildModelAsync, and call base.BuildModelAsync() to customize your model.
///</remarks>
public class RestierModelBuilder : IModelBuilder 
{
    ///<summary>
    /// The ODataConventionModelBuilder instance that will contain the generated model.
    ///</summary>
    public ODataConventionModelBuilder ModelBuilder { get; set; }

    ///<summary>
    /// Creates a new instance of the RestierModelBuilder.
    ///</summary>
    /// <param name="namespace">The namespace to use for the ModelBuilder.</param>
    public RestierModelBuilder(string namespace)
    {
        ModelBuilder = new ODataConventionModelBuilder();
        ModelBuilder.Namespace = namespace;
    }

    ///<summary>
    /// Please start documenting these methods so that Intellisense works.
    </summary>
    public async Task BuildModelAsync(ModelContext context, CancellationToken cancellationToken)
    {
        ModelBuilder.AutomagicStuffHere();
        ....
    }

    ///<summary>
    /// Returns an EdmModel instance from the ModelBuilder.
    ///</summary>
    public IEdmModel GetEdmModel() => return ModelBuilder.GetEdmModel(); 

}

Then you don't need virtual methods, you don't need chains, you don't need anything unusual... just register your custom inherited ModelBuilder with the DI container before you call MapRestierRoute() in WebApiConfig.cs. Just like any other DI user would expect.

IMO, any place where you are having to call InnerXXX is a candidate for this type of cleaner, simplified architecture. And with this structure, 100% of the code used to build existing WebAPI OData models will be compatible with RESTier.

HTH!

This comment has been minimized.

@chinadragon0515

chinadragon0515 Jun 21, 2016

Contributor

@advancedrei thanks for your great comments, first let's see what we want to achieve,

User need to build a model, and here are what we need to build,

  1. Some entity sets and entity types and these are from DbContext (For some user who does not need this part, he will not have this part). (Done by RestierModelBuilder with data set in ModelContext by EfModelBuilder)
  2. More entity sets and entity types been built besides the ones from DbContext (small group of consumers) (User need to add something here, this is what we simplify with this PR)
  3. Entity sets and types built by end user who does not use EF (must for people who does not use EF). (Already support, as inner most service)
  4. Restier build additional entity sets, singletons defined in Api class (done by RestierModelExtender)
  5. Restier build operation model directly from method defined in Api class (done by RestierOperationModelBuilder).
  6. User may customize/log/add more element to the model (Already support, as out most service)

Also we want to allow user to use any ways to build model which means user can build with OData lib Edm class directly or with Web Api OData conversion model build.

Then let discuss the design point,

  1. For users who use EF, we expect they do not need to touch model build at all, all are done by restier model builder.
  2. For users who need additional entity sets and entity types or who does not use EF (refer to item 2), we allow them to extend, but we want to be extend more easily, then his does not need to touch operation model build.
  3. For users who may customize/log/add more element to the model, we allow him to customize and most he need to do with model directly like set a new annotation.

So we plan to return model directly, but user can create a new conversion model builder with existing model, we open issue OData/WebApi#755

You provide a great option to achieve this, we may extract existing logic into different methods and user can call these method in some specified orders. We will have more discussions on this part.

This comment has been minimized.

@robertmclaws

robertmclaws Jun 21, 2016

Collaborator

Just to make sure I point it out, if you put all of your existing awesomeness into the ildModelAsync() function, it will construct the model for you by default. Anyone needing to customize the process would just call base.BuildModelAsync() in their override code, and then finish with their modifications. That way, people that add functions, actions, or non-entity types to their APIs can do so after the existing model has been built. Also, this method would make it really easy to add things like ignored properties and what-not, without having to take over building the entire model, like you have to do right now.

It would look like this:

///<summary>
/// Handles the automagic process of constructing a ModelBuilder instance from a given DbContext.
///</summary>
public class CustomRestierModelBuilder : RestierModelBuilder 
{
    ///<summary>
    /// Creates a new instance of the CustomRestierModelBuilder.
    ///</summary>
    /// <param name="namespace">The namespace to use for the ModelBuilder.</param>
    public CustomRestierModelBuilder(string namespace) : base(namespace)
    {
    }

    ///<summary>
    /// Please start documenting these methods so that Intellisense works.
    </summary>
    public override async Task BuildModelAsync(ModelContext context, CancellationToken cancellationToken)
    {
        base.BuildModelAsync(context, cancellationToken);

        // My custom stuff here
        ModelBuilder.EntitySet<Contact>("Contacts").EntityType.Ignore(c => c.IsDeleted);
        ModelBuilder.Action("SomeAction").Returns<SomeAwesomeType>();
        ....
    }

}

For the end user, this will be so much cleaner than the implementation you have now. Super-simple.

Expecting that EF people won't need to touch the model is a bad assumption. Especially if using the attributes to get Actions to route properly is as difficult for everyone else as it was for me this past weekend.

I wouldn't extract stuff into different methods right now. Keep It Simple. Don't over-engineer it. The whole point of the framework is to make building these APIs ridiculously simple. You have to be ruthless with the design simplicity, and don't engineer features no one is asking for because few are using it. Right. @markdstafford? :)

This comment has been minimized.

@chinadragon0515

chinadragon0515 Jul 5, 2016

Contributor

@advancedrei I have discussions with Mark and undo the model extend part, we will use issue #457 to track model extend part, we plan to hide chain from end user when they want to extend the model.

@chinadragon0515 chinadragon0515 changed the title from Add bound flag and make model extend more easier to Add bound flag and make namespace consistent Jul 1, 2016

@chinadragon0515 chinadragon0515 merged commit 265736c into OData:master Jul 5, 2016

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