fix: encode URL params correctly for SourceId in Cargo.lock

[weihanglo]

What does this PR try to resolve?

Fixes #11085, also as a part of #12120.

When serialize Resolve for Cargo.lock, previously Cargo relys on SourceId::as_url, which is good for user-facing string. However, it leads to an incorrect deserialization since url crate always assume the given string is correctly url-encoded.

cargo/src/cargo/core/source/source_id.rs

Lines 527 to 538 in 0d5370a

	impl ser::Serialize for SourceId {
	fn serialize<S>(&self, s: S) -> Result<S::Ok, S::Error>
	where
	S: ser::Serializer,
	{
	if self.is_path() {
	None::<String>.serialize(s)
	} else {
	s.collect_str(&self.as_url())
	}
	}
	}

This fixes by adding a wrapper for encoding SourceId during the serialization for Cargo.lock.

How should we test and review this PR?

This is a conitnuation of #12279. #12279 should be reviewed first.

We use form_urlencoded::byte_serialize, which is re-exported by url crate. Tests are copied from #11086. Kudos to the original author!

[rustbot]

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

[ehuss]

I'm a little uncertain about having different serializations of SourceId. Can you say more about why it wouldn't have the same form as what is in Cargo.lock? I'm a little concerned about having them out of sync, though I don't have a specific scenario where that would be an issue.

(Out of sync in the sense of cargo metadata or JSON messages (or any other messages?) having different forms than Cargo.lock.)

[weihanglo]

My original thought was to keep the human-readable message as good as possible. That was my mistake not thinking too much on cargo metadata and JSON messages.

Now I lean toward making all of them use URL encode, except lockfile version 3. Old lockfiles will remain in the “wrong” format to avoid unnecessary churns. Do you think this new approach would be a compatibility hazard for cargo metadata and others?

[ehuss]

Do you think this new approach would be a compatibility hazard for cargo metadata and others?

No. I don't think the package ID's and such have stability guarantees.

[ehuss]

I think I'd like to go ahead and kick of an fcp to merge this, though not strictly necessary since this is still unstable. I think we should be extra cautious about lockfile changes. Also, please confirm my rehashing of the issue.

@rfcbot fcp merge

@rust-lang/cargo This is an unstable change to Cargo.lock encoding. This will be part of the version 4 encoding, which is currently unstable and can only be used with -Znext-lockfile-bump on nightly. We may accumulate other changes to roll up in the version 4 change to reduce churn (described in #12120).

For a rollout, eventually in the future we will stabilize the ability for cargo to read the version 4 lock files, but cargo won't create them. Eventually, after at least 6 months after that, cargo will start writing version 4 lock files when creating them from scratch. Otherwise, lock file versions are sticky, and they aren't updated automatically (which we may want to reconsider, at least for this specific case).

This change involves changing the serialization of git dependencies. Git dependencies are encoded as a URL in the lock file:

[[package]]
name = "regex"
version = "1.8.4"
source = "git+https://github.com/rust-lang/regex.git?branch=branchname#5a34a39b72d85730065d3ffe4ce3715f2731e49a"

Unfortunately the serialization code blindly includes the git branch, tag, or revision as a string in the URL without properly escaping the contents (with % encoding). But the deserialization is expecting a URL and does proper decoding. This results in a few different problems:

• A + symbol gets changed to a space. When cargo loads that, it sees ?branch=foo+bar does not match ?branch=foo bar and assumes it has changed, forcing it to fetch the git repo and rewrite Cargo.lock every time.
• A # symbol confuses the fragment parser. ?branch=foo#bar#SHA1 is interpreted as branch=foo. This causes cargo to think Cargo.lock is out of date (because the branch is actually foo#bar), and re-fetches the git repo, and rewrites Cargo.lock (again with the broken #).
• A % symbol is interpreted as % encoding when it shouldn't. If the next two letters are hex-like, then it will decode them. For example, foo%bar would be decoded as foo�r, causing the same refetch/rewrite cycle as mentioned above.

We can't just change this without a lock version bump, because older versions of Cargo will see the % encoding and convert it back to the non-% encoding, overwrite Cargo.lock, and start the cycle of refetching and updating Cargo.lock.

[rfcbot]

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Eh2406
• @Muscraft
• @arlosi
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

Concerns:

• consistency resolved by fix: encode URL params correctly for SourceId in Cargo.lock #12280 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[ehuss]

@rfcbot concern consistency

Raising a concern to have SourceId serialized consistently in JSON messages to match that in Cargo.lock mentioned at #12280 (comment). I'm not sure how feasible that is to do properly. For example, should it be dependent on the current version of Cargo.lock? Or should it always use the new format? For simplicity, I would lean towards always using the new format, but I'm uncertain about what considerations are needed here.

[weihanglo]

No. I don't think the package ID's and such have stability guarantees.

Does that mean user should see id and source file in cargo metadata output as opaque strings? If this is the case, should we document it?

concern consistency

Agree with ehuss on always using the new format. If we see it as a bug, it should be fine just fixing JSON message and SourceId serialization together. We don't do that for Cargo.lock because it may mess up version control systems.

However, maybe we shouldn't change until we are going to stabilize lockfile v4? That could make things more consistent.

[ehuss]

Does that mean user should see id and source file in cargo metadata output as opaque strings? If this is the case, should we document it?

I was thinking be essence that it is specified as a "unique ID" and nothing else that it is otherwise not defined. It might be good to explicitly say it is opaque (I did include that in the unit graph docs).

However, maybe we shouldn't change until we are going to stabilize lockfile v4? That could make things more consistent.

That seems fine with me. Is there some way we can track that so it isn't forgotten? Maybe just in #12120?

[weihanglo]

That seems fine with me. Is there some way we can track that so it isn't forgotten? Maybe just in #12120?

Does that mean this pull request is good to go if we can remember set them to true when stabilizing?

BTW, just added a new commit for reminding us inline in source code. Also left a comment there in #12120 (comment).

[ehuss]

Seems good enough to resolve my concern.

@rfcbot resolve consistency

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[ehuss]

Thanks!

@bors r+

[bors]

📌 Commit 273285f has been approved by ehuss

It is now in the queue for this repository.

[bors]

⌛ Testing commit 273285f with merge 7c82ff2...

[bors]

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 7c82ff2 to master...