Rework remote settings server URL #6429

bendk · 2024-10-24T15:10:26Z

Let's update these to match the JS client:
https://github.com/mozilla-extensions/remote-settings-devtools/blob/15245fdb8e4fb5875a7866c7328ded6671ab466c/extension/experiments/remotesettings/api.js#L8-L11

This is a breaking change to the public API, but this API is not exposed via UniFFI and it's only used in 1 place:

application-services/components/nimbus/src/stateful/client/mod.rs

Lines 24 to 35 in 50bf235

    
           let url = remote_settings_server.url()?; 
        
           if url.scheme() == "file" { 
        
               // Everything in `config` other than the url/path is ignored for the 
        
               // file-system - we could insist on a sub-directory, but that doesn't 
        
               // seem valuable for the use-cases we care about here. 
        
               let path = match url.to_file_path() { 
        
                   Ok(path) => path, 
        
                   _ => return Err(NimbusError::InvalidPath(url.into())), 
        
               }; 
        
               Box::new(FileSystemClient::new(path)?) 
        
           } else { 
        
               Box::new(RemoteSettings::new(config)?)

. AFAICT, the only affect this will have is that the error message will change slightly.

Also removed the .unwrap() calls. It shouldn't matter because there's no way for .join() to fail, but it feels silly calling unwrap() from a function that returns a result.

Pull Request checklist

Breaking changes: This PR follows our breaking change policy
- This PR follows the breaking change policy:
  - This PR has no breaking API changes, or
  - There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

codecov-commenter · 2024-10-24T15:20:05Z

Codecov Report

Attention: Patch coverage is 0% with 43 lines in your changes missing coverage. Please review.

Project coverage is 21.76%. Comparing base (8bdea9d) to head (f807f09).

Files with missing lines	Patch %	Lines
components/remote_settings/src/client.rs	0.00%	34 Missing ⚠️
components/remote_settings/src/config.rs	0.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6429      +/-   ##
==========================================
- Coverage   21.78%   21.76%   -0.02%     
==========================================
  Files         344      344              
  Lines       31028    31053      +25     
==========================================
  Hits         6759     6759              
- Misses      24269    24294      +25

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

leplatrem

I'm sorry if it wasn't ultra clear in other messages, but our convention is to not put the trailing / in the server url and build paths with {server-url}/buckets/...

(Note: I didn't put comments everywhere this has to be adjusted)

leplatrem · 2024-10-25T09:41:33Z

components/remote_settings/src/client.rs

@@ -122,24 +122,29 @@ pub trait ApiClient {

 /// Client for Remote settings API requests
 pub struct ViaductApiClient {
+    /// Server base URL
+    ///
+    /// This is something like `https://[domain]/v1/`.  It's what's normally called the `base_url`


Our convention is to not put the trailing /

leplatrem · 2024-10-25T09:41:52Z

components/remote_settings/src/client.rs

    /// Base URL for requests to a collections endpoint
    ///
    /// This is something like
-    /// `https://[server-url]/v1/buckets/[bucket-name]/collections/[collection-name]/"
+    /// `https://[server-url]/buckets/[bucket-name]/collections/[collection-name]/`


This is correct (only if no trailing slash in server-url)

leplatrem · 2024-10-25T09:42:19Z

components/remote_settings/src/client.rs

    collection_url: Url,
    remote_state: RemoteState,
 }

 impl ViaductApiClient {
    fn new(server_url: Url, bucket_name: &str, collection_name: &str) -> Result<Self> {
        let collection_url = server_url.join(&format!(
-            "v1/buckets/{bucket_name}/collections/{collection_name}/"
+            "buckets/{bucket_name}/collections/{collection_name}/"


Suggested change

"buckets/{bucket_name}/collections/{collection_name}/"

"/buckets/{bucket_name}/collections/{collection_name}/"

I avoided this because it's going to break the join. Without the trailing slash, then join treats the last component as a file rather than a directory and removes it. However, there is the path_segments_mut() method. Let me try that and see how it comes out.

bendk · 2024-10-25T15:18:52Z

components/remote_settings/src/client.rs

+        let mut server_url = base_url.clone();
+        // Push the empty string to add a trailing slash.  This avoids a redirect when we hit
+        // the server.
+        Self::path_segments_mut(&mut server_url)?.push("");


Using https://firefox.settings.services.mozilla.com/v1/ instead of https://firefox.settings.services.mozilla.com/v1 seems right to me. I always try to avoid hitting a redirect when I make an API call. But tell me if that's a bad idea.

There should not be any redirect, we don't call the server_url directly (without trailing slash)

bendk · 2024-10-25T15:19:56Z

Your v1/ vs v1 suggestion pushed me to make a bunch of changes and I'm liking the result. Encapsulating the URL creating into a single class seems like the nicest way to handle this.

leplatrem

This looks good, and approved to avoid blocking you longer.

Nevertheless, in order to avoid confusion between the different implementations, I would like to insist that we shouldn't put any trailing slash to the server URL.

I agree with you, it would be ugly to hit a redirect, but we don't because we consider / as the root endpoint.

So I think that with your PR, if we just add a root endpoint to your struct, we should be aligned with the other implementations 😊

Examples in kinto-http.py:

class Endpoints(object):
    endpoints = {
        "root": "{root}/",
        "batch": "{root}/batch",
        "buckets": "{root}/buckets",
        "bucket": "{root}/buckets/{bucket}",
        "history": "{root}/buckets/{bucket}/history",
        "groups": "{root}/buckets/{bucket}/groups",
        "group": "{root}/buckets/{bucket}/groups/{group}",
        "collections": "{root}/buckets/{bucket}/collections",
        "collection": "{root}/buckets/{bucket}/collections/{collection}",
        "records": "{root}/buckets/{bucket}/collections/{collection}/records",  # NOQA
        "record": "{root}/buckets/{bucket}/collections/{collection}/records/{id}",  # NOQA
    }

or kinto.js

const ENDPOINTS = {
  root: () => "/",
  bucket: (bucket?: string) => "/buckets" + (bucket ? `/${bucket}` : ""),
  collection: (bucket: string, coll?: string) =>
    `${ENDPOINTS.bucket(bucket)}/collections` + (coll ? `/${coll}` : ""),
  record: (bucket: string, coll: string, id?: string) =>
    `${ENDPOINTS.collection(bucket, coll)}/records` + (id ? `/${id}` : ""),
    // ...
    // ...
};

leplatrem · 2024-10-28T17:02:06Z

components/remote_settings/src/client.rs

-                let collection_url = self.collection_url.clone();
-                let server_info = self.make_request(collection_url)?.json::<ServerInfo>()?;
+                let server_info = self
+                    .make_request(self.endpoints.server_url.clone())?


I would call it root_url

leplatrem · 2024-10-28T17:03:02Z

components/remote_settings/src/client.rs

+    /// Top-level URL for Remote Settings server
+    ///
+    /// This has the form `https://[domain]/v1/`. It's where we get the attachment base url from.
+    server_url: Url,


In other implementations, we call this the root URL and it has the form of {server_url}/

leplatrem · 2024-10-28T17:05:55Z

components/remote_settings/src/client.rs

+        let mut server_url = base_url.clone();
+        // Push the empty string to add a trailing slash.  This avoids a redirect when we hit
+        // the server.
+        Self::path_segments_mut(&mut server_url)?.push("");


There should not be any redirect, we don't call the server_url directly (without trailing slash)

Let's update these to match the JS client: https://github.com/mozilla-extensions/remote-settings-devtools/blob/15245fdb8e4fb5875a7866c7328ded6671ab466c/extension/experiments/remotesettings/api.js#L8-L11 This is a breaking change to the public API, but this API is not exposed via UniFFI and it's only used in 1 place: https://github.com/mozilla/application-services/blob/50bf235b9a3a62a6b6749cd0bd7358d7b96a42e4/components/nimbus/src/stateful/client/mod.rs#L24-L35. AFAICT, the only affect this will have is that the error message will change slightly. Fixed an error when downloading attachments in the new client code. I think I broke it with my last commit. Also removed the `.unwrap()` calls. It shouldn't matter because there's no way for `.join()` to fail, but it feels silly calling `unwrap()` from a function that returns a result.

bendk · 2024-10-28T21:11:01Z

That makes sense. I updated the terminology to be root_url. That actually removed all references to server_url.

bendk requested review from leplatrem and a team October 24, 2024 15:10

bendk force-pushed the push-owyotmxrznxy branch 2 times, most recently from 4837c02 to 966b7da Compare October 24, 2024 17:27

leplatrem requested changes Oct 25, 2024

View reviewed changes

bendk force-pushed the push-owyotmxrznxy branch 3 times, most recently from a181293 to 2a80bc1 Compare October 25, 2024 15:17

bendk commented Oct 25, 2024

View reviewed changes

bendk force-pushed the push-owyotmxrznxy branch from 2a80bc1 to 0cbe484 Compare October 25, 2024 15:24

leplatrem approved these changes Oct 28, 2024

View reviewed changes

bendk force-pushed the push-owyotmxrznxy branch from 0cbe484 to 781b59c Compare October 28, 2024 21:08

bendk force-pushed the push-owyotmxrznxy branch from 781b59c to f807f09 Compare October 28, 2024 21:10

bendk enabled auto-merge October 28, 2024 21:17

bendk added this pull request to the merge queue Oct 28, 2024

Merged via the queue into mozilla:main with commit dbc8e8d Oct 28, 2024
16 checks passed

bendk deleted the push-owyotmxrznxy branch October 28, 2024 22:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework remote settings server URL #6429

Rework remote settings server URL #6429

bendk commented Oct 24, 2024

codecov-commenter commented Oct 24, 2024 •

edited

Loading

leplatrem left a comment •

edited

Loading

leplatrem Oct 25, 2024 •

edited

Loading

leplatrem Oct 25, 2024

leplatrem Oct 25, 2024

bendk Oct 25, 2024

bendk Oct 25, 2024

leplatrem Oct 28, 2024

bendk commented Oct 25, 2024 •

edited

Loading

leplatrem left a comment •

edited

Loading

leplatrem Oct 28, 2024

leplatrem Oct 28, 2024

leplatrem Oct 28, 2024

bendk commented Oct 28, 2024

	let url = remote_settings_server.url()?;
	if url.scheme() == "file" {
	// Everything in `config` other than the url/path is ignored for the
	// file-system - we could insist on a sub-directory, but that doesn't
	// seem valuable for the use-cases we care about here.
	let path = match url.to_file_path() {
	Ok(path) => path,
	_ => return Err(NimbusError::InvalidPath(url.into())),
	};
	Box::new(FileSystemClient::new(path)?)
	} else {
	Box::new(RemoteSettings::new(config)?)

	"buckets/{bucket_name}/collections/{collection_name}/"
	"/buckets/{bucket_name}/collections/{collection_name}/"

Rework remote settings server URL #6429

Rework remote settings server URL #6429

Conversation

bendk commented Oct 24, 2024

Pull Request checklist

codecov-commenter commented Oct 24, 2024 • edited Loading

Codecov Report

leplatrem left a comment • edited Loading

Choose a reason for hiding this comment

leplatrem Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendk commented Oct 25, 2024 • edited Loading

leplatrem left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendk commented Oct 28, 2024

codecov-commenter commented Oct 24, 2024 •

edited

Loading

leplatrem left a comment •

edited

Loading

leplatrem Oct 25, 2024 •

edited

Loading

bendk commented Oct 25, 2024 •

edited

Loading

leplatrem left a comment •

edited

Loading