-
Notifications
You must be signed in to change notification settings - Fork 161
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
Fixes #1286: String Collections support in $select. #1282
Open
anasik
wants to merge
36
commits into
OData:release-8.x
Choose a base branch
from
anasik:fix/primitive-collections
base: release-8.x
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
36 commits
Select commit
Hold shift + click to select a range
857aefb
Checking for primitive collection in SelectExpandBinder.ProjectAsWrap…
anasik ea34a17
Checking only for string since that seems to be the only case that's …
anasik 3c2ab5e
tests hehe
anasik 79727fa
Added SQLite DB to aid with testing; Discovered that all collections …
anasik c126703
Added target project, excluded db file from compilation, made test co…
anasik cfd9ae0
Added DateOnly and put #if_NET8 around the if block. Also removed the…
anasik 411cf73
removed commented code
anasik 7fc15a8
Uncorrupt
anasik 5c12e84
git attributes, last ditch effort to fix sqlite db
anasik 85be658
Forgot to rename in some places
anasik 2c9a286
db again fingers crossed
anasik 77e9ec0
Removed toList from controller since the bug pertains to deferred exe…
anasik e638c4d
Commented out DateTimeOffset filter test cases since the error itself…
anasik d9c2a05
Update src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandB…
anasik 8d1b843
Update test/Microsoft.AspNetCore.OData.E2E.Tests/Lists/ListsDataModel.cs
anasik e4ea1d8
Update test/Microsoft.AspNetCore.OData.E2E.Tests/Lists/ListsControlle…
anasik ac53066
renamed ListTestOrders to ListTestOrder for consistency
anasik e75b803
Init order
anasik 629a7b9
ADDED tests for ListTestOrder
anasik b780b77
TypeHelper.isPrimitive
anasik 5946b66
Orders table
anasik 861beda
Using direct table reference instead of Set. Dropping Orders table in…
anasik 380c97a
Added Orders to Context
anasik ed8b203
Added Orders to EDM
anasik c086e15
Added ListTestOrder to _map and $expand to both test cases
anasik 8d617fa
removed unnecessary spaces
anasik e41fb01
Update src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandB…
anasik 706f1eb
Renamed function. Added TimeOnly.
anasik 2275cc2
Added Decimal
anasik 6cbec7a
Return doc.
anasik d8ae66e
Update src/Microsoft.AspNetCore.OData/Common/TypeHelper.cs
anasik 6105b07
Remove duplicated doc
anasik 18ce012
Added copyright comment to ListsContext and removed commented lines f…
anasik 0039e0a
Removed *.db file in favor of in-memory SQLite
anasik 41ebcb0
removed db binary gitattribute
anasik 167b3e8
fixed indentation
anasik File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
26 changes: 26 additions & 0 deletions
26
test/Microsoft.AspNetCore.OData.E2E.Tests/Lists/ListsContext.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
//----------------------------------------------------------------------------- | ||
// <copyright file="ListsContext.cs" company=".NET Foundation"> | ||
// Copyright (c) .NET Foundation and Contributors. All rights reserved. | ||
// See License.txt in the project root for license information. | ||
// </copyright> | ||
//------------------------------------------------------------------------------ | ||
|
||
using System; | ||
using System.IO; | ||
using Microsoft.EntityFrameworkCore; | ||
|
||
anasik marked this conversation as resolved.
Show resolved
Hide resolved
|
||
namespace Microsoft.AspNetCore.OData.E2E.Tests.Lists | ||
{ | ||
public class ListsContext : DbContext | ||
{ | ||
public ListsContext(DbContextOptions<ListsContext> options) | ||
: base(options) | ||
{ | ||
|
||
} | ||
|
||
public DbSet<Product> Products{ get; set; } | ||
public DbSet<Order> Orders{ get; set; } | ||
} | ||
} | ||
|
186 changes: 186 additions & 0 deletions
186
test/Microsoft.AspNetCore.OData.E2E.Tests/Lists/ListsController.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,186 @@ | ||
//----------------------------------------------------------------------------- | ||
// <copyright file="ListsController.cs" company=".NET Foundation"> | ||
// Copyright (c) .NET Foundation and Contributors. All rights reserved. | ||
// See License.txt in the project root for license information. | ||
// </copyright> | ||
//------------------------------------------------------------------------------ | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using Microsoft.AspNetCore.Http; | ||
using Microsoft.AspNetCore.Mvc; | ||
using Microsoft.AspNetCore.OData.Deltas; | ||
using Microsoft.AspNetCore.OData.Formatter; | ||
using Microsoft.AspNetCore.OData.Query; | ||
using Microsoft.AspNetCore.OData.Routing.Attributes; | ||
using Microsoft.AspNetCore.OData.Routing.Controllers; | ||
using Microsoft.EntityFrameworkCore; | ||
using Xunit; | ||
|
||
namespace Microsoft.AspNetCore.OData.E2E.Tests.Lists | ||
{ | ||
[Route("convention")] | ||
public class ProductsController : ODataController | ||
{ | ||
private readonly ListsContext _dbContext; | ||
public ProductsController(ListsContext context) | ||
{ | ||
_dbContext = context; | ||
} | ||
|
||
[EnableQuery(PageSize = 10, MaxExpansionDepth = 5)] | ||
public IActionResult Get() | ||
{ | ||
return Ok(_dbContext.Products); | ||
} | ||
|
||
[EnableQuery] | ||
public IActionResult Get(string key) | ||
{ | ||
return Ok(_dbContext.Products.Find(key)); | ||
} | ||
|
||
public IActionResult Post([FromBody] Product Product) | ||
{ | ||
Product.ProductId = _dbContext.Products.Count() + 1+""; | ||
_dbContext.Products.Add(Product); | ||
_dbContext.SaveChanges(); | ||
|
||
return Created(Product); | ||
} | ||
|
||
public IActionResult Put(int key, [FromBody] Product Product) | ||
{ | ||
Product.ProductId = key+""; | ||
Product originalProduct = _dbContext.Products.Find(key); | ||
|
||
if (originalProduct == null) | ||
{ | ||
_dbContext.Products.Add(Product); | ||
|
||
return Created(Product); | ||
} | ||
|
||
_dbContext.Products.Remove(originalProduct); | ||
_dbContext.Products.Add(Product); | ||
return Ok(Product); | ||
} | ||
|
||
public IActionResult Patch(int key, [FromBody] Delta<Product> delta) | ||
{ | ||
Product originalProduct = _dbContext.Products.Find(key); | ||
|
||
if (originalProduct == null) | ||
{ | ||
Product temp = new Product(); | ||
delta.Patch(temp); | ||
_dbContext.Products.Add(temp); | ||
return Created(temp); | ||
} | ||
|
||
delta.Patch(originalProduct); | ||
return Ok(delta); | ||
} | ||
|
||
public IActionResult Delete(int key) | ||
{ | ||
Product Product = _dbContext.Products.Find(key); | ||
|
||
_dbContext.Products.Remove(Product); | ||
return this.StatusCode(StatusCodes.Status204NoContent); | ||
} | ||
|
||
[HttpPost("ResetDataSource")] | ||
public IActionResult ResetDataSource() | ||
{ | ||
_dbContext.Orders.RemoveRange(_dbContext.Orders); | ||
_dbContext.Products.RemoveRange(_dbContext.Products); | ||
_dbContext.SaveChanges(); | ||
|
||
// Add new seed data | ||
_dbContext.Products.AddRange( | ||
new Product | ||
{ | ||
ProductId = "1", | ||
Name = "Product1", | ||
Category = "Category1", | ||
ListTestString = new List<string> { "Test1", "Test2", "Test99" }, | ||
ListTestBool = new List<bool> { true, false }, | ||
ListTestInt = new List<int> { 1, 2, 3 }, | ||
ListTestDouble = new List<double> { 1.1, 2.2 }, | ||
ListTestFloat = new List<float> { 1.1f, 2.2f }, | ||
ListTestDateTime = new List<DateTimeOffset> { DateTime.Now, DateTime.UtcNow }, | ||
ListTestUri = new List<Uri> { new Uri("https://example.com") }, | ||
ListTestUint = new uint[] { 1, 2, 3 }, | ||
ListTestOrder = new List<Order> | ||
{ | ||
new Order { OrderId = "Order1" }, | ||
new Order { OrderId = "Order2" } | ||
} | ||
}, | ||
new Product | ||
{ | ||
ProductId = "2", | ||
Name = "Product2", | ||
Category = "Category2", | ||
ListTestString = new List<string> { "Test3", "Test4", "Test99" }, | ||
ListTestBool = new List<bool> { false, true }, | ||
ListTestInt = new List<int> { 4, 5, 6 }, | ||
ListTestDouble = new List<double> { 3.3, 4.4 }, | ||
ListTestFloat = new List<float> { 3.3f, 4.4f }, | ||
ListTestDateTime = new List<DateTimeOffset> { DateTime.Now.AddDays(1), DateTime.UtcNow.AddDays(1) }, | ||
ListTestUri = new List<Uri> { new Uri("https://example.org") }, | ||
ListTestUint = new uint[] { 4, 5, 6 } | ||
}, | ||
new Product | ||
{ | ||
ProductId = "3", | ||
Name = "Product3", | ||
Category = "Category3", | ||
ListTestString = new List<string> { "Test5", "Test6" }, | ||
ListTestBool = new List<bool> { true, true }, | ||
ListTestInt = new List<int> { 7, 8, 9 }, | ||
ListTestDouble = new List<double> { 5.5, 6.6 }, | ||
ListTestFloat = new List<float> { 5.5f, 6.6f }, | ||
ListTestDateTime = new List<DateTimeOffset> { DateTime.Now.AddDays(2), DateTime.UtcNow.AddDays(2) }, | ||
ListTestUri = new List<Uri> { new Uri("https://example.net") }, | ||
ListTestUint = new uint[] { 7, 8, 9 } | ||
}, | ||
new Product | ||
{ | ||
ProductId = "4", | ||
Name = "Product4", | ||
Category = "Category4", | ||
ListTestString = new List<string> { "Test98", "Test98" }, | ||
ListTestBool = new List<bool> { false, false }, | ||
ListTestInt = new List<int> { 10, 11, 12 }, | ||
ListTestDouble = new List<double> { 7.7, 8.8 }, | ||
ListTestFloat = new List<float> { 7.7f, 8.8f }, | ||
ListTestDateTime = new List<DateTimeOffset> { DateTime.Now.AddDays(3), DateTime.UtcNow.AddDays(3) }, | ||
ListTestUri = new List<Uri> { new Uri("https://example.edu") }, | ||
ListTestUint = new uint[] { 10, 11, 12 } | ||
}, | ||
new Product | ||
{ | ||
ProductId = "5", | ||
Name = "Product5", | ||
Category = "Category5", | ||
ListTestString = new List<string> { "Test98", "Test98" }, | ||
ListTestBool = new List<bool> { true, false }, | ||
ListTestInt = new List<int> { 13, 14, 15 }, | ||
ListTestDouble = new List<double> { 9.9, 10.10 }, | ||
ListTestFloat = new List<float> { 9.9f, 10.10f }, | ||
ListTestDateTime = new List<DateTimeOffset> { DateTime.Now.AddDays(4), DateTime.UtcNow.AddDays(4) }, | ||
ListTestUri = new List<Uri> { new Uri("https://example.gov") }, | ||
ListTestUint = new uint[] { 13, 14, 15 } | ||
} | ||
); | ||
_dbContext.SaveChanges(); | ||
return this.StatusCode(StatusCodes.Status204NoContent); | ||
} | ||
|
||
|
||
} | ||
|
||
} |
36 changes: 36 additions & 0 deletions
36
test/Microsoft.AspNetCore.OData.E2E.Tests/Lists/ListsDataModel.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
//----------------------------------------------------------------------------- | ||
// <copyright file="ListsDataModel.cs" company=".NET Foundation"> | ||
// Copyright (c) .NET Foundation and Contributors. All rights reserved. | ||
// See License.txt in the project root for license information. | ||
// </copyright> | ||
//------------------------------------------------------------------------------ | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.ComponentModel.DataAnnotations; | ||
using System.Runtime.Serialization; | ||
|
||
namespace Microsoft.AspNetCore.OData.E2E.Tests.Lists | ||
{ | ||
public class Product | ||
{ | ||
[Key] | ||
public string ProductId { get; set; } | ||
public string Name { get; set; } | ||
public string Category { get; set; } | ||
public IList<string> ListTestString { get; set; } = new List<string>(); | ||
public IList<bool> ListTestBool { get; set; } | ||
public IList<int> ListTestInt { get; set; } | ||
public IList<double> ListTestDouble { get; set; } | ||
public IList<float> ListTestFloat { get; set; } | ||
public IList<DateTimeOffset> ListTestDateTime { get; set; } | ||
public IList<Uri> ListTestUri { get; set; } | ||
public uint[] ListTestUint { get; set; } | ||
public IList<Order>? ListTestOrder { get; set; } | ||
} | ||
anasik marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
public class Order | ||
WanjohiSammy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
[Key] public string OrderId { get; set; } | ||
} | ||
} |
28 changes: 28 additions & 0 deletions
28
test/Microsoft.AspNetCore.OData.E2E.Tests/Lists/ListsEdmModel.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
//----------------------------------------------------------------------------- | ||
// <copyright file="ListsEdmModel.cs" company=".NET Foundation"> | ||
// Copyright (c) .NET Foundation and Contributors. All rights reserved. | ||
// See License.txt in the project root for license information. | ||
// </copyright> | ||
//------------------------------------------------------------------------------ | ||
|
||
using Microsoft.OData.Edm; | ||
using Microsoft.OData.ModelBuilder; | ||
|
||
namespace Microsoft.AspNetCore.OData.E2E.Tests.Lists | ||
{ | ||
internal class ListsEdmModel | ||
{ | ||
public static IEdmModel GetConventionModel() | ||
{ | ||
ODataConventionModelBuilder builder = new ODataConventionModelBuilder(); | ||
EntitySetConfiguration<Product> Products = builder.EntitySet<Product>("Products"); | ||
EntitySetConfiguration<Order> Orders = builder.EntitySet<Order>("Orders"); | ||
|
||
builder.Namespace = typeof(Product).Namespace; | ||
builder.Namespace = typeof(Order).Namespace; | ||
|
||
var edmModel = builder.GetEdmModel(); | ||
return edmModel; | ||
} | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The primitive types covered here are not sufficient. You can refer to IsPrimitiveType(Type clrType) in ODL which contains all the types that we consider primitive both
Nullable
orNon-Nullable
primitive types. Also looking at this method, you will notice that we do not considerUri
primitive type.Please use it to rewrite this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WanjohiSammy, I am well-aware of the correct primitive types but I had to cover all the types that are not only affected by the bug but were also documented to support Primitive Collections in the EF Core 8 change log.
That being said, I took a look at said method it's not immediately evident to me exactly which types of mine you're concerned about. It appears that the only difference between that method and mine is that I explicitly added cases for some types that the other function might still support.
I am almost certain that there's not a single type mentioned there that wasn't first confirmed to be affected.
Since you've already mentioned Uri,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the confusion here stems from the fact that these two methods have similar names but serve very different purposes. The core
IsPrimitiveType()
test for types that can be mapped to EDM primitive types. But this method checks for types that should be treated as primitive collections in the context of EF core, without regards to EDM types. Maybe using a different name, more specific to this use case would alleviate the confusion. But I think it's fine to restrict this method to only the types that matter with respect to the EF core bug.