OData / AspNetCoreOData

ASP.NET Core OData: A server library built upon ODataLib and ASP.NET Core
Other
457 stars 158 forks source link

Code convention used for parameters in controller methods inconsistent with Entity Framework Code Convention #155

Open FlippieCoetser opened 3 years ago

FlippieCoetser commented 3 years ago

By convention, key should be used as method parameter in Controller :

Current

        [EnableQuery]
        public IActionResult Get(int key)
        {
            return Ok(_context.Entities.FirstOrDefault(entity => entity.Id == key));
        }

Response image

Improvement

Support the use of Id as a parameter in controller methods to ensure consistency between System.ComponentModel.DataAnnotations and Microsoft.AspNetCore.OData. See Entity Framework Core Code Convention and #152

Therefore, the below method should produce the same results as in the response above.

        [EnableQuery]
        public IActionResult Get(int id)
        {
            return Ok(_context.Entities.FirstOrDefault(entity => entity.Id == id));
        }
FlippieCoetser commented 3 years ago

@xuzhg I have been looking into a possible approach to enable both key and id as discussed above.

From what I can see it looks like an explicit check is done in Microsoft.AspNetCore.OData.Extensions.ActionModelExtensions

        public static bool HasODataKeyParameter(this ActionModel action, IEdmEntityType entityType, string keyPrefix = "key")
        {
            if (action == null)
            {
                throw Error.ArgumentNull(nameof(action));
            }

            if (entityType == null)
            {
                throw Error.ArgumentNull(nameof(entityType));
            }

            // TODO: shall we make sure the type is matching?
            var keys = entityType.Key().ToArray();
            if (keys.Length == 1)
            {
                // one key
                return action.Parameters.Any(p => p.ParameterInfo.Name == keyPrefix);
            }
            else
            {
                // multipe keys
                foreach (var key in keys)
                {
                    string keyName = $"{keyPrefix}{key.Name}";
                    if (!action.Parameters.Any(p => p.ParameterInfo.Name == keyName))
                    {
                        return false;
                    }
                }

                return true;
            }
        }

Looking at all the references I never see the method called with a keyPrefix passed in: the method is always invoked using the default value key.

My question: Would it be acceptable to refactor the method to check for both key or id? What do you think? Are there any other methods I should consider or would a simple pull request do?