Skip to content

Add support for identifiers nested properties.#1

Open
Feiryn wants to merge 1 commit intoJenChampagne:masterfrom
Feiryn:nested-properties
Open

Add support for identifiers nested properties.#1
Feiryn wants to merge 1 commit intoJenChampagne:masterfrom
Feiryn:nested-properties

Conversation

@Feiryn
Copy link
Contributor

@Feiryn Feiryn commented Sep 13, 2024

Allows the use of nested properties, such as: address/city. I also allowed single character identifiers.

@Feiryn
Copy link
Contributor Author

Feiryn commented Oct 3, 2024

Hello @JenChampagne, do you accept PRs on this project?

@JenChampagne
Copy link
Owner

JenChampagne commented Oct 24, 2024

Yes, contributions are welcome and appreciated. Apologies for not responding sooner.

I'm going to review the OData spec and think a little on this, but I'll make sure to come back by the weekend to not leave this dangling too long. Once I return, I'll create and publish a new version of the crate.

Overall I think this is a positive improvement and I do want to support this. I'm wondering though whether the internal type of String in Expr::Identifier should also be changed. My understanding of the spec is that . operators in identifiers represent nested identifiers, and that the / operators in identifiers represent foreign identifiers.

For example, /Customers?$filter=Address.City eq 'Toronto' would refer to the nested property of City within the column Address of the table Customers. I think there could still be some benefit to pre-splitting Address and City rather than making users of this crate have to always look for and handle special characters. So at a minimum, I think there could be a convenience gained by a type adjustment here.

In contrast to the previous example, /Orders?$filter=Customer/Address.City eq 'London' would refer to the nested property of City within the column Address of the table Customers table, where the row in the Customer table is selected through a join to the table Orders. This has so much more context than a simple identifier, and so it's possible this might deserve it's own Expr variant, or probably that Identifier needs to contain a complex type where it could be a regular identifier, a nested identifier, a foreign table identifier, or a foreign table nested identifier.

I don't want to suggest any specific recommendation until I'm sure what I think will be best long-term. The above are my thoughts at the moment.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants