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

Convert output to serde_json::Value before printing #98

Closed
wants to merge 1 commit into from

Conversation

smoelius
Copy link
Contributor

@smoelius smoelius commented Nov 2, 2024

This change causes map keys to be ordered alphabetically. An example appears below.

I'm not 100% sure of the reason for the difference. I suspect that without converting to a value, serde_json uses Serde machinery to traverse struct fields (e.g., PublisherData) in the order in which they are declared. Whereas, a serde_json::Objects's keys are always ordered alphabetically.

I would be happy to add a test. I would just need some direction on where an how to add it.


Without the change:

$ cargo supply-chain json --diffable
...
    "xdg": [
      {
        "id": 654,
        "login": "whitequark",
        "kind": "user",
        "name": "whitequark",
        "avatar": "https://avatars1.githubusercontent.com/u/54771?v=4"
      }
    ]
...

With the change:

$ cargo run -- supply-chain json --diffable
...
    "xdg": [
      {
        "avatar": "https://avatars1.githubusercontent.com/u/54771?v=4",
        "id": 654,
        "kind": "user",
        "login": "whitequark",
        "name": "whitequark"
      }
    ]
...

@Shnatsel
Copy link
Member

Shnatsel commented Nov 2, 2024

I believe it's more efficient to rearrange struct fields to keep them alphabetical and serialize the data like that, instead of going through the serde_json::Value machinery. It would probably be a better idea to do that, if we can enforce it without too much trouble.

But why is an alphabetical ordering desirable in the first place?

@smoelius
Copy link
Contributor Author

smoelius commented Nov 2, 2024

But why is an alphabetical ordering desirable in the first place?

To make diffing more predictable.

My guess about why things are ordered they way they currently are is just a guess. I haven't found anything in serde_json to explain the current behavior.

Also, FWIW, converting output to a serde_json::Value has other differences. For example, it moves not_audited to after crates_io_crates.

@Shnatsel
Copy link
Member

Shnatsel commented Nov 2, 2024

Could you provide a specific example where it helps? Like, a specific tool, and show the diffs before and after?

I'd like to understand the exact effect of the changes to determine the best course of action.

@smoelius
Copy link
Contributor Author

smoelius commented Nov 2, 2024

Here are the complete "before" and "after" for cargo-supply-chain itself:
before.json
after.json

I think having any imposed order helps. I haven't found it in the serde_json's documentation, but there is this comment from dtolnay:

Yeah, as long as you don't enable preserve_order then Value will always print with map keys sorted lexicographically.

Based on my understanding, there are no guarantees regarding the order of keys coming from non-Values, which makes diffing unpredictable.

Also, apologies. I should have opened an issue to rather than assume this change would be accepted.

@Shnatsel
Copy link
Member

Shnatsel commented Nov 2, 2024

In practice the fields are printed in the declaration order. I understand that the fields are printed in a stable order, even if that order may appear arbitrary. So I don't see how this change would improve diffing.

I was hoping you could show me the JSON diffs before and after this change, and the diffs after would be clearly better somehow.

@smoelius
Copy link
Contributor Author

smoelius commented Nov 2, 2024

I was hoping you could show me the JSON diffs before and after this change, and the diffs after would be clearly better somehow.

I don't think I can provide that.

Thanks for your consideration, and thanks for a great tool, which I use in several of my projects!

@smoelius smoelius closed this Nov 2, 2024
@Shnatsel
Copy link
Member

Shnatsel commented Nov 2, 2024

Thank you for taking the time to open a PR anyway. And I'm glad you find the tool useful!

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