Skip to content
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

Kiota should send properties in OAS declaration order, not alphabetic order #4680

Closed
bkoelman opened this issue May 18, 2024 · 12 comments
Closed

Comments

@bkoelman
Copy link

bkoelman commented May 18, 2024

What are you generating using Kiota, clients or plugins?

API Client/SDK

In what context or format are you using Kiota?

Nuget tool

Client library/SDK language

Csharp

Describe the bug

My OAS file lists properties (in component schema) in an intentional order: type, id, firstName, lastName, age. While the last three vary per model, the first two are always present. Kiota ignores that and uses alphabetic order instead: age, firstName, id, lastName, type. This makes the payload harder to read, test, and debug.

Expected behavior

Preserve declaration order. In the case of inheritance, fields from base types come first.

How to reproduce

Open API description file

No response

Kiota Version

1.14.0

Latest Kiota version known to work for scenario above? (Not required)

No response

Known Workarounds

None

Configuration

No response

Debug output

No response

Other information

No response

@bkoelman bkoelman added status:waiting-for-triage An issue that is yet to be reviewed or assigned type:bug A broken experience labels May 18, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Kiota May 18, 2024
@baywet baywet added the status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close label May 20, 2024
@baywet
Copy link
Member

baywet commented May 20, 2024

Hi @bkoelman
Thanks for using kiota and for reaching out.
Can you outline further why do you think this makes things harder to debug?
The reasons why properties are in alphabetical order it to:

  • get deterministic output
  • ease debug/code reading (easier/faster to scan an ordered list than an unordered one)

@bkoelman
Copy link
Author

The declaration order in OAS is already deterministic. All other code generators I've tried respect the declaration order.
Suppose a list of entities is returned, this:

[
  { "type": "user", "id": "1", "firstName": "John", "lastName": "Doe", "age": 25 },
  { "type": "order", "id": "1", "productName": "Laptop", "quantity": 1, "price": 1199.99, "units": "dollars" },
  { "type": "order", "id": "2", "productName": "Printer", "quantity": 1, "price": 349.99, "units": "dollars" }
]

is much easier to read than:

[
  { "age": 25, "firstName": "John", "id": "1", "lastName": "Doe", "type": "user" },
  { "id": "1", "price": 1199.99, "productName": "Laptop", "quantity": 1, "type": "order", "units": "dollars" },
  { "id": "2", "price": 349.99, "productName": "Printer", "quantity": 1, "type": "order", "units": "dollars" }
]

Lastly, specific implementations may depend on the ordering. Why else would System.Text.Json and Newtonsoft provide ways to control the order, other than to improve readability?

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels May 20, 2024
@baywet
Copy link
Member

baywet commented May 20, 2024

Thanks for the additional information.
I'm not sure why anybody would implement an API with dependencies on the order of properties for a JSON payload when the JSON RFC especially calls out object properties are unordered by nature.

An object is an unordered collection of zero or more name/value
pairs, where a name is a string and a value is a string, number,
boolean, null, object, or array.

This sounds like a bad design of the API itself that should be addressed in the API instead of pushing a constraint on clients. Do you have example scenarios that cannot be achieved through any other way than a specific ordering of the properties?

@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels May 20, 2024
@bkoelman
Copy link
Author

Not at the moment, but years ago I ran into that with XML more than once: Large commercial enterprise systems that we couldn't change because they were provided by external vendors, or because the source code got lost over time (yes, that happens, and more often than you'd think, even today). Or 3rd party external systems written in PHP, that didn't rely on an XML DOM parser, because at the time there wasn't one built-in yet. We had to work around these problems because getting the external systems changed was too expensive.

I agree that an implementation that depends on the order is a bad design, however, sometimes it's out of our control. But my primary concern is readability, as my sample shows.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels May 20, 2024
@baywet
Copy link
Member

baywet commented May 20, 2024

Thanks for the additional information.
I want to be transparent with the fact that we're unlikely to invest in ordering to accommodate the legacy/badly designed systems.

As for the readability aspect, is it because you want the discriminator property to be first? or am I missing something here?

@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels May 20, 2024
@bkoelman
Copy link
Author

bkoelman commented May 20, 2024

It's unrelated to discriminators, it's about preserving the declaration order. A model that defines first name, middle name and last name (in that order) should not be hussled.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels May 20, 2024
@baywet
Copy link
Member

baywet commented May 20, 2024

I understand you have a specific opinion of how things should be, however between the JSON spec that explicitly calls out the unordering, and lack of additional evidence/scenarios, it's only an opinion.
I'm going to go ahead and close this issue since it's unlikely we're going to change things here.
Thanks for your understanding.

@baywet baywet closed this as not planned Won't fix, can't repro, duplicate, stale May 20, 2024
@github-project-automation github-project-automation bot moved this from Needs Triage 🔍 to Done ✔️ in Kiota May 20, 2024
@baywet baywet added question and removed type:bug A broken experience Needs: Attention 👋 status:waiting-for-triage An issue that is yet to be reviewed or assigned labels May 20, 2024
@bkoelman
Copy link
Author

That's ridiculous. SQL columns are unordered in SELECT, yet their order is preserved in EF Core queries. C# methods are unordered, yet their order is preserved in IL (MetadataToken). Not because it's a technical requirement, but because it improves usability and readability. Even IDictionary<,> preserves insertion order.

I can understand this is low priority, but closing this because your personal opinion goes against what's common in most other tools, and ignoring all motivation, is just disrespectful. You're apparently not very experienced with developer expectations, nor managing an open source project. It would make more sense to leave this open for others to upvote. I've proven you wrong on nullability before, which is now the most updated issue. Having some faith in what users care about would avoid a lot of frustration. This recurring attitude of "I know better than the rest of the world" makes me eager to abandon kiota, and I'm pretty sure more people feel that way.

@baywet
Copy link
Member

baywet commented May 20, 2024

We have countless instances of issues being left open for upvote/future consideration in this repo. (see v2 or v3 or backlog milestones for examples, or even past versions). This includes multiple of your own issues

At some point, the sheer management of issues being left open becomes a challenge in itself, this is why I drive the conversations to demonstrate tangible additional values. You've had multiple opportunities to demonstrate that "if we invest in making sure properties are ordered in the same way they are in the description, we unblock this scenario, which is likely to benefit a broad base of users", and you haven't.

@darrelmiller
Copy link
Member

darrelmiller commented May 20, 2024

@bkoelman The JSON specification specifications states https://datatracker.ietf.org/doc/html/rfc8259#section-4

JSON parsing libraries have been observed to differ as to whether or
not they make the ordering of object members visible to calling
software. Implementations whose behavior does not depend on member
ordering will be interoperable in the sense that they will not be
affected by these differences.

As Kiota is intended to support any JSON parsing library, taking dependencies on the behaviour of some libraries would impact interoperability. As far as developer experience goes, I've yet to see an autocomplete experience that lists members of an object in declared order. Using alphabetical ordering is quite common when selecting properties.

The OpenAPI.NET library that Kiota is based upon makes no attempt to preserve document content that is outside of the OpenAPI semantics. e.g. map ordering, whitespace, Therefore it would make it challenging to consistently carry through this information into Kiota and project it into source code.

It was not a design objective of Kiota use non-significant formatting of the source OpenAPI as part of code generation. I have not seen strong reasoning in this thread to suggest revisiting that goal. I do think there is value in having a deterministic order of the properties.

I would be open to considering using semantics like required/readonly/etc to suggest ordering, but attempting to insult community members doesn't seem like the right approach to make that case.

@bkoelman
Copy link
Author

bkoelman commented May 22, 2024

We have countless instances of issues being left open for upvote/future consideration in this repo. (see v2 or v3 or backlog milestones for examples, or even past versions). This includes multiple of your own issues

I appreciate the effort to act and decide on issues early, instead of leaving them lingering. Exceptions aside, the large amount of open issues reflects the poor quality of kiota. Because most are bug reports, or clumsy limitations that make using kiota a pain (from a consumer perspective). It sounds like you're doing me a favor by not closing my bug reports? When the goal is building a flourishing community, it should be the other way around: be grateful for opening them, acknowledging that other people are giving up their free time to provide an opportunity to build a better product, even when the team currently doesn't have time for that. Having many open bugs reflects the product is popular, which may give you leverage in allocating more resources to the team. That, and not bothering to test with the provided repro steps, and closing issues as completed without actually fixing the problem (taking note isn't enough; taking responsibility means reopening, and converting it to an umbrella issue that lists other issues it depends on), is what I meant with being unexperienced in managing an open source project.

At some point, the sheer management of issues being left open becomes a challenge in itself, this is why I drive the conversations to demonstrate tangible additional values.

I don't see why. GitHub has great capabilities to manage large amounts of issues, by using tags, milestones, projects, advanced issue filters, etc. Runtime has 5k open issues, aspnetcore has 3k open issues; perhaps ask there on how to manage them?

You've had multiple opportunities to demonstrate that "if we invest in making sure properties are ordered in the same way they are in the description, we unblock this scenario, which is likely to benefit a broad base of users", and you haven't.

I've given several motivations, which you haven't responded to. You're supposed to explain why your point of view is the better solution, and what the downsides are in my point of view. Just dismissing them isn't having a discussion.

As Kiota is intended to support any JSON parsing library, taking dependencies on the behaviour of some libraries would impact interoperability. As far as developer experience goes, I've yet to see an autocomplete experience that lists members of an object in declared order. Using alphabetical ordering is quite common when selecting properties.

Neither does the spec recommend to order them alphabetically, which is what kiota does. I'm just asking to not sort them, I have no idea what kind of investment you're referring to. Sorting the list of schemas is fine, but the model properties reflect something functional that users care about. It matters in reading logged requests and responses. Autocomplete is a lookup tool that has its own logic to sort, based on the most-likely match. The declaration order is irrelevant in that case, but that was never my point.

The OpenAPI.NET library that Kiota is based upon makes no attempt to preserve document content that is outside of the OpenAPI semantics. e.g. map ordering, whitespace, Therefore it would make it challenging to consistently carry through this information into Kiota and project it into source code.

I'm not concerned about whitespace, but OpenAPI.NET doesn't take explicit steps to change the ordering, which is nice. I'd be fine if kiota worked similarly.

It was not a design objective of Kiota use non-significant formatting of the source OpenAPI as part of code generation. I have not seen strong reasoning in this thread to suggest revisiting that goal. I do think there is value in having a deterministic order of the properties.

Why would this tradeoff be different for kiota than for OpenAPI.NET?

I would be open to considering using semantics like required/readonly/etc to suggest ordering

I don't see the advantage of that. I'd consider that unintuitive.

but attempting to insult community members doesn't seem like the right approach to make that case.

That's not my intent. In being a fully-remote worker for years I've learned to always assume best intent, asking for clarification instead of making assumptions. But also to reach out if things aren't right. As expressed above, there wasn't much of a discussion. Perhaps some of you are just too overworked, but feedback and concerns are often dismissed without explanation, which is demotivating and disrespectful. These and other concerns came up in #4349, but I'm not seeing a change in attitude. There's an underlying sentiment of disliking feedback and not being interested in what matters to users, despite saying otherwise.

@bkoelman
Copy link
Author

Here's evidence that users care about the order: dotnet/runtime#102932:

While this is consistent with the semantics of JSON where order of properties is not significant, it is often the case that one wants to arrange the order out of human readability concerns. For example, it is common for JSON schema docs to place the $schema and $id properties at the start of the object.

Another reason is performance: in System.Text.Json, a JsonConverter takes a forward-only reader. In JSON:API the "type" property determines how the remaining properties are serialized. When it doesn't appear at the top, the converter needs to run a second pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

3 participants