feat(literal-types): including all literal types for non-navigation/simple members

[NetanelPersik]

feat(literal-types): including all literal types for non-navigation/simple members:

e.g.

• Dictionaries<string, int>
• int[]
• Nested: Dictionary<List<>, Dictionary<string, int>>

[BlaiseD]

1. Can you say which PostgreSQL types we would be supporting? Your test will pass for KeyValuePair<int, string> not just lists and dictionaries.

2. It's also easier to reverse changes if the update is something like:

            return parentType.GetMemberInfos().Where
            (
                info => (info.MemberType == MemberTypes.Field || info.MemberType == MemberTypes.Property)
                && (info.GetMemberType().IsLiteralType() || info.GetMemberType().IsLiteralList() || info.GetMemberType().AllUnderlyingTypesLiteral())
            ).ToArray();

3. You're returning the type itself from UnderlyingElementTypes() in place of the call to IsLiteralType() - also harder to reverse.

[NetanelPersik]

1. I think it is up to the developer to decide which structure their DB supports, in my case, since OData doesn't natively support $filter on dictionaries, I want to use EF Value Conversation to treat dictionaries as List<KeyValuePair<,>>

2. // 3. Not sure I understand your concern, is it about rollback the changes in case of a regression/bug? If so, can't we create a rollback PR to the current PR?.
   In addition, since AllUnderlyingTypesLiteral() is also covering Lists I don't see a point keeping IsLiteralList()

[NetanelPersik]

3.) note that I'm calling . AllUnderlyingTypesLiteral() which returns a boolean, not the type itself (it used .All that calls UnderlyingElementTypes)

[BlaiseD]

What you've written is GenericTypeOne<int, string, int, GenericTypeTwo<string, int, int, string>> which is any object graph.

Sounds like you need GenericTypeOne to be an IEnumerable with the with arg[0] on the underlying types as a literal type? What generic arguments does the PastgreSQL dictionary allow?

I wasn't referring to operationally reverting a commit. Keeping those 3 groups of literal types separate makes it easier to address/see/understand a problem with any one of them. AnyLiteralTypes does the reverse.

[NetanelPersik]

Postgresql allows jsonb column type, which can be represented as numerous types in c# as long as you have proper mapping, e.g. jsonb can be represented as Dictionary<string, string> , List<KeyValuePair<,>> plain string etc... and even GenericTypeOne<int, string, int, GenericTypeTwo<string, int, int, string>>. My point is to let the consumer of the library deal with it

As for the readability, I'm not sure I follow, IMHO keeping only AllUnderlyingTypesLiteral with proper UT should be sufficient, it is kind of confusing to have info.GetMemberType().IsLiteralType() || info.GetMemberType().IsLiteralList() || info.GetMemberType().AllUnderlyingTypesLiteral() it sounds like AllUnderlyingTypesLiteral should cover them all, WDYT?

[BlaiseD]

Ideally, we want to expand all non-navigation properties by default. Literal lists and literals etc.. approximate that behavior but leave out a few properties. It's probably better to copy the expansion logic to AutoMapper.AspNetCore.OData and use OData's metadata to identify non-navigation properties. IEdmEntityType has a NavigationProperties() extension method. Here's a similar use of metadata.

Agree?

[NetanelPersik]

Should the logic be in QueryableExtensions?

[BlaiseD]

I think a new class is better - maybe ExpansionsHelper.

[NetanelPersik]

Resolved as part of AutoMapper/AutoMapper.Extensions.OData#229