json feature affects serde_json configuration

[jovfer]

I'm trying to use sqlx with json feature enabled in the same project where serde_json is already in use.

Enabling json feature in sqlx finally results in preserve_order activated for serde_json for the whole project. And it may be unacceptable for some cases. https://github.com/launchbadge/sqlx/blob/v0.4.2/Cargo.toml#L90 https://github.com/launchbadge/sqlx/blob/v0.4.2/sqlx-macros/Cargo.toml#L67

What is the reason to force preserve_order in serde_json? Can it be not forced?

I have a draft for potential fix: separate feature without forcing preserve_order. https://github.com/jovfer/sqlx/commit/0f58bd756ecdd2e683c6e866a322ce22c5719561 Please let me know if it may be useful and I should raise a PR.

[abonander]

It's turned on in the macros, for a semi-stupid reason. When deserializing the sqlx-data.json it expects the db key to come first. I'm not entirely sure the preserve_order feature is necessary for this, however. Go ahead and open the PR, but please test with cargo sqlx prepare and SQLX_OFFLINE=1 as I don't think we cover that flow in CI.

[mehcode]

Shouldn't the new feature resolver in Cargo fix this? It isolates the build graph for proc-macro crates.

[jovfer]

@abonander thanks for clarification

@mehcode sounds interesting, I'm not familiar with the latest changes in Cargo. I'll take a look on release notes and try..

[vimmerru]

@mehcode

Shouldn't the new feature resolver in Cargo fix this? It isolates the build graph for proc-macro crates

According to cargo documentation new resolver allows

* Ignores features for target-specific dependencies for targets that don't match the current compile target.
* Prevents features enabled on build dependencies or proc-macros from being enabled for normal dependencies. 
* Prevents features enabled on dev dependencies from being enabled for normal dependencies.

I don't see anything about different build graphs. Most probable it will not help. I tried to build with resolver=2 and nightly compiler, but behavior is the same.

When deserializing the sqlx-data.json it expects the db key to come first. I'm not entirely sure the preserve_order feature is necessary for this

As i understand you are talking about https://github.com/launchbadge/sqlx/blob/master/sqlx-cli/src/prepare.rs . I quickly checked the code and don't see anything related to ordering of json keys. I plan send PR with just removed preserve_ordering feature and try to additionally test sqlx-cli.

Update

• Just noticed that sqlx-macros is proc-macro crate. In this case building with host_dep cargo feature may help in distant future. Anyway documentation says

Note that proc-macro decoupling requires changes to the registry, so it won't be decoupled until the registry is updated to support the new field.

Seems It should work after 1.51 rust release, but will require resolver = "2" option in Cargo.toml

[vimmerru]

See https://github.com/launchbadge/sqlx/pull/1071

[abonander]

I quickly checked the code and don't see anything related to ordering of json keys.

Specifically, this bit of the actual query macros code that looks for the query data in sqlx-data.json: https://github.com/launchbadge/sqlx/blob/master/sqlx-macros/src/query/data.rs#L158-L163

This code is designed to avoid scanning the whole file by stopping once it's found the db key and the key corresponding to the hash for the query string of the current macro invocation. If it doesn't see the db key before finding the hash key it's looking for, then it will return an error.

This implicit assumption of ordering should be fine as we serialize with a struct which should mean the ordering is the same as the field definitions: https://github.com/launchbadge/sqlx/blob/master/sqlx-cli/src/prepare.rs#L20-L24

However, when I wrote that code I was concerned that #[serde(flatten)] may futz with the ordering because I'd seen in generated code that it may sometimes collect fields to a map internally, although it turns out this is only done when deserializing a flattened field, and it uses a Vec.

So my concerns were likely unfounded but that's why I asked that anyone attempting it to be sure to test it because we don't test the offline feature in CI very well (to my knowledge).