Adding IndexMap under feature flag preserve_order

[ralpha]

WIP: Do not merge yet
Implementation for #32

This is only a small change.
Most of the change is to make impl<K, V> Merge for Map<K, V> work again.
Because the Extens<(K, V)> for IndexMap<K, V, S> implementation relays on K: Hash + Eq + Copy, V: Copy, As Copy is not always implemented for things like String and most structs. So I make a small implementation of extend in there. With insert that only needs to relays K: std::hash::Hash + Eq. The Ord trait was already there. Just moved it down.

Tested

I tested the code both in the okapi example and my code. Both work with and without the feature flag set.

Import in code

When testing make sure you point okapi,rocket-okapi and your own code to the same Schemars version.

Example in my code (and okapi example): (Cargo.toml)

# My normal way of importing schemars
schemars = "0.7"
# With feature flag pointing to my local copy.
schemars = { version = "0.7.0", 
path = "path_to_somewhere/schemars/schemars", features = ["preserve_order"] }
# Without the feature flag.
schemars = { version = "0.7.0", 
path = "path_to_somewhere/schemars/schemars", features = [] }

Weird API call list order

The only odd behavior that I have noticed is that the order of the api calls is not consistent.
Order without feature flag:

Order with flag run first time:

Close and run again:

This is different every time you close and start the server again (does not even have to recompile).
I do not know how this comes. But also have not looked into this yet.
The order in the rocket output is always the same.
I have only seen this with the routes not the other parts.
Order changes in json as you can see here (both with feature flag, just closed and started server in between):

Works, proof

But here is proof that is works in other cases:

With flag:

Feedback

So what do you think? What can be changes/improved. You maybe have an idea where the order bug comes from? Maybe just timing or multi-threading?

@ThouCheese are you maybe willing to give this a try in your project to see if you spot any compatibility error?

[GREsau]

The inconsistent ordering may be caused by this usage of hashmap in okapi:
https://github.com/GREsau/okapi/blob/18a30b7353b8748f23844fbe6c56694b64536773/rocket-okapi/src/gen.rs#L17

...which should probably be an okapi::Map (equivalent to schemars::Map) instead of a hashmap

[ralpha]

The inconsistent ordering may be caused by this usage of hashmap in okapi:
https://github.com/GREsau/okapi/blob/18a30b7353b8748f23844fbe6c56694b64536773/rocket-okapi/src/gen.rs#L17

...which should probably be an okapi::Map (equivalent to schemars::Map) instead of a hashmap

Some limited testing seems to confirm this.
Re implemented it as Map first. But that does not work because BTreeMap.
the trait bound rocket_http::method::Method: std::cmp::Ord is not satisfied
So I made a type AltMap<K, V> that implements HashMap if flag is not set and IndexMap when flag is set.

Also had to do expose the Entry for each Map.
See commit.

[ralpha]

In rocket-okapi changed

// Old
use okapi::Map;
// New
use schemars::{Map, AltMap, AltMapEntry};

And used AltMap and AltMapEntry in lines below.
This seems to fix the ordering problem.
Will add documentation later. (When we are getting ready to merge his)
Other remarks?

[GREsau]

Rather than introducing AltMap, I'd rather change the usage of HashMap (which currently causes okapi to be nondeterministic) to BTreeMap/IndexMap.

This would require making the key implement Ord - I think one way of doing this would be to change the key from (String, Method) to (String, &'static str), using as_str() to convert the method to its string representation.

[GREsau]

Other options for replacing HashMap<(String, Method), Operation> with:

• schemars::Map<String, HashMap<Method, Operation>>
• schemars::Map<String, PathItem>

[ralpha]

I removed MapAlt again because of your recent comment.
I changed my internal implementation to use the schemars::Map<String, HashMap<Method, Operation>> option. But I have to say that my familiarity with the codebase is not that good that I know how this totally effected the codebase. So I think @GREsau can best decode what is the best solution.

Just for reference here are the other changes I made to other file/crates to implement these changes. Note: This will change everything over to use the preserve_order flag. If you want to give users more options the feature flag has to be the same in all implementation to avoid weird behavior.
okapi/Cargo.toml

schemars = { version = "0.7", features = ["preserve_order"] }

rocket_okapi/Cargo.toml

schemars = { version = "0.7", features = ["preserve_order"] }
# Maybe add option to okapi to use features flag?
okapi = { version = "0.4", features = ["preserve_order"]  }

rocket_okapi/src/gen.rs

...
use schemars::{Map, MapEntry};
...
use std::collections::HashMap;
...
pub struct OpenApiGenerator {
    settings: OpenApiSettings,
    schema_generator: SchemaGenerator,
    operations: Map<String, HashMap<Method, Operation>>,
}
...
    pub fn add_operation(&mut self, mut op: OperationInfo) {
        if let Some(op_id) = op.operation.operation_id {
            // TODO do this outside add_operation
            op.operation.operation_id = Some(op_id.trim_start_matches(':').replace("::", "_"));
        }
        match self.operations.entry(op.path) {
            MapEntry::Occupied(e) => {
                let path = e.key();
                panic!("An OpenAPI operation has already been added for {}", path);
            }
            MapEntry::Vacant(e) => {
                let mut map = HashMap::new();
                map.insert(op.method, op.operation);
                e.insert(map)
            }
        };
    }
...
    pub fn into_openapi(self) -> OpenApi {
        OpenApi {
            openapi: "3.0.0".to_owned(),
            paths: {
                let mut paths = Map::new();
                for (path, map) in self.operations {
                    for (method, op) in map {
                        let path_item = paths.entry(path.clone()).or_default();
                        set_operation(path_item, method, op);
                    }
                }
                paths
            },
            ....
            ..Default::default()
        }
    }

I also noted that rocket_okapi_codegen/src/openapi_attr/mod.rs uses BTreeMap. It does not seem to effect the result that I have noted but just mentioning. (this crate does not depend on schemars or okapi)

In a project I would then use it as:
Cargo.toml

schemars = { version = "0.7", features = ["preserve_order"] }
# optional change (if okapi further forward this feature)
okapi = { version = "0.4", features = ["preserve_order"] }

So okapi and rocket_okapi have to incorporate this change too to take effect.

For the changed to this crate there is not much else to do I think. I encountered no problems while testing. I think this can be merged not and we can continue the conversation further in https://github.com/GREsau/okapi
Once schemars is update I'll create a pull request there (if needed, all changes should be in the code above). This will make the merge easier to create and test.

[GREsau]

Thanks for the PR, I've merged it after a making couple of tweaks. IndexMap's Extend implementation actually doesn't require the key/value to be Copy so I could keep the simpler implementation of Merge.