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

Stop sorting of schema properties #32

Closed
ralpha opened this issue May 22, 2020 · 4 comments
Closed

Stop sorting of schema properties #32

ralpha opened this issue May 22, 2020 · 4 comments

Comments

@ralpha
Copy link
Contributor

ralpha commented May 22, 2020

Currently the schema properties are sorted alphabetical.
This might be desired in some cases. But could you allow an option to use the order of the fields in the structure?

This is handy because my id is now somewhere in the middle and when you have responses with a lot of optional fields this is very confusing. And it does not line up with how the actual response comes back.

Looking at this code (it might not be the right part of the code). You are using a Map that is defined as:

/// The map type used by schemars types.
///
/// Currently a `BTreeMap`, but this may change a different implementation
/// with a similar interface in a future version of schemars.
pub type Map<K, V> = std::collections::BTreeMap<K, V>;

pub properties: Map<String, Schema>,

We can maybe replace that with Vec<(String, Schema)> but then you have to add some searching by key.

The other option is adding some kind of order indicator to Schema. But then Serde Serialize will still not sort the list by that id.

There might also be other options.

So I think the first option is the better one.
I could help with this implementation if you want to.

@GREsau
Copy link
Owner

GREsau commented May 24, 2020

I expected this enhancement to be requested at some point, which is the main reason that type alias was introduced, so that they underlying type could easily be swapped out!

My planned fix for this is to optionally enable the use of IndexMap in place of BTreeMap depending on a feature flag, which would probably be called preserve_order to be consistent with serde-json:
https://github.com/serde-rs/json/blob/master/Cargo.toml#L53-L56

@GREsau
Copy link
Owner

GREsau commented Jun 7, 2020

This should now be fixed in v0.8.0-alpha-2 - thanks for the PR!

I just need to finish some documentation and cleanup and then 0.8.0 proper should be good to go

@GREsau GREsau closed this as completed Jun 7, 2020
@phiresky
Copy link

phiresky commented Jun 9, 2020

Great to see this already fixed! Was wondering why my properties were all mixed up. But it should probably be the default in my opinion, since I don't think you'd usually care much about the performance of this, but you do care very much about the order of properties etc...

@ralpha
Copy link
Contributor Author

ralpha commented Jun 9, 2020

I agree with setting this as the default. Although getting it working in okapi should be done first I think. This will make sure it is working correctly in all cases.
GREsau/okapi#31

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

No branches or pull requests

3 participants