EdmLocation has no usable properties.

[chrisspre]

IEdmLocatable is declared as returning an EdmLocation which is an abstract type without a property. which essentially requires to cast the returned value to something concrete.

https://github.com/OData/odata.net/blob/4e263ca4b94d01f2d105d8282c62ec272cfcd1bb/src/Microsoft.OData.Edm/Schema/Interfaces/IEdmLocatable.cs

https://github.com/OData/odata.net/blob/4e263ca4b94d01f2d105d8282c62ec272cfcd1bb/src/Microsoft.OData.Edm/EdmLocation.cs

All reasonable implementations of this type should have at least a line number but potentially all the properties of CsdlLocation

Assemblies affected

Microsoft.OData.Edm Version="7.9.0" and before

[habbes]

Hi @chrisspre we had a discussion around this and @xuzhg said that the EdmLocation is an abstract concept and does not make any assumptions about the "physical" location of the object being located. CsdlLocation has line numbers because it's associated with a CSDL file, and line numbers make sense in that context. But EdmLocation is not tied to a particular context. @xuzhg could you please share more of your thoughts here as well.

My view is that if the EdmLocation is so abstract that it cannot contain any properties, then it's not any more useful than the base object type. I think there should be at least some common member (other than ToString() which is already in object) that is common to all location implementations.

Alternatively, maybe a generic type would be more suitable in this case, that way each implementation of IEdmLocatable could defined its own location implementation, i.e.:

interface IEdmLocatable<TLocation> {
    TLocation Location;
}

Then you could use it without casting:

CsdlLocatable: IEdmLocatable<CsdlLocation>`

But this would be a major breaking change.

[chrisspre]

Hi @chrisspre we had a discussion around this and @xuzhg said that the EdmLocation is an abstract concept and does not make any assumptions about the "physical" location of the object being located. CsdlLocation has line numbers because it's associated with a CSDL file, and line numbers make sense in that context. But EdmLocation is not tied to a particular context. @xuzhg could you please share more of your thoughts here as well.

Yes, understood and no objection to that. Problem is that it means there is not a lot of use for EdmLocation, and one needs to know when and how to cast to CsdlLocation to make it useful.

My view is that if the EdmLocation is so abstract that it cannot contain any properties, then it's not any more useful than the base object type. I think there should be at least some common member (other than ToString() which is already in object) that is common to all location implementations.

Agreed

Alternatively, maybe a generic type would be more suitable in this case, that way each implementation of IEdmLocatable could defined its own location implementation, i.e.:

interface IEdmLocatable<TLocation> {
    TLocation Location;
}

Then you could use it without casting:

CsdlLocatable: IEdmLocatable<CsdlLocation>`

But this would be a major breaking change.

Definitely agree with he statement about breaking change. The intro of the generic type might also require changes in other types.

Bu I don't see much harm in introducing a property on EdmLocation that has an OPTIONAL file location value (a value with path, line, column). It is so prevalent that IEdmLocatables actually are loaded from a file that this often has a value, and if not, code deals with the fact that it is empty. (which it also has to do when casting to CsdlLocation)

[corranrogue9]

I am adding context from one of our investigations meetings to each of the EDM related work items. Please keep these scenarios in mind when investigating this work item:

It has been suggested (by myself) that removing the EDM interfaces and replacing them with concrete types where appropriate would be a good idea. 2 existing scenarios were enumerated for why the interfaces are necessary, 1 existing scenario was enumerated for why the interfaces are helpful (but not strictly necessary), 1 known future scenario was enumerated for why the interaces continue to be necessary, and 1 potential future scenario was enumerate for why the interfaces provide existensibility that wouldn't be available with concrete types.

Existing necessary scenarios

1. The interfaces are used today for mocking in unit tests
2. The interfaces could be used by customers to provide custom implementations for each type

Existing helpful scenarios

1. It is helpful to have a read-only view into the EDM models

Known future scenario

1. We want to allow customers to update their models after, for example, the model has been read from a CSDL file. The interfaces being a read-only view give the option for the concrete types to be used for post-read modifications, with the interfaces being used once those modifications are complete.

Potential future scenario

1. There is a potential scenario where the full model being loaded into memory is not necessary for a customer. With the current structure of the IEdmModel interface, this means that, if all other EDM types were concrete types, then any instance of IEdmModel would necessarily need to load portions of the model into memory even if those portions aren't used. A new implementation of non-model EDM interfaces would allow for this even though the concrete types would not.

[mikepizzo]

It has been suggested (by myself) that removing the EDM interfaces and replacing them with concrete types where appropriate would be a good idea.

The other reason for using interfaces is to be able to support multiple inheritance. Can we model the current hierarchy without multiple inheritance?