address PR review feedback on the Connect Cloud auth refactor

docs/CHANGELOG.md

@@ -8,21 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## Unreleased
 
 - Posit Connect Cloud is now a supported deployment target, alongside Posit
-  Connect and shinyapps.io, mirroring the R rsconnect package's support.
-  Register an account with `rsconnect add -n <nickname> --connect-cloud
-  -A <account>`, which verifies the account exists and that you can publish to
-  it. Authentication is an interactive browser login by default; for CI and
-  other non-interactive use, pass a service account credential with
-  `--client-id`/`--client-secret` (or the `CONNECT_CLOUD_CLIENT_ID` and
-  `CONNECT_CLOUD_CLIENT_SECRET` environment variables). Tokens are refreshed
-  automatically. Deploy with any `rsconnect deploy` subcommand by passing
-  `--connect-cloud` (or `-s connect.posit.cloud`) with `-A <account>` (or the
-  `CONNECT_CLOUD_ACCOUNT` environment variable; a `SHINYAPPS_ACCOUNT` variable
-  exported for shinyapps.io is ignored here), or `-n <nickname>` for a saved
-  credential — which publishes to the account it was saved with, or to another
-  account of the same login when `-A` is given as well. Supported content types:
-  Shiny (Python and R), Streamlit, Dash, Bokeh, Jupyter notebooks, Quarto,
-  R Markdown, and static content.
+  Connect and shinyapps.io.
 - `rsconnect add` now reports invalid option combinations and unreadable
   certificate files as plain error messages. Previously these surfaced as raw
   Python tracebacks.

rsconnect/api.py

@@ -177,7 +177,7 @@ def handle_bad_response(self, response: HTTPResponse | T, is_httpresponse: bool
                         response.json_data["error"],
                     )
                     raise RSConnectException(error, status=response.status)
-                if response.status < 200 or response.status > 299:
+                if response.status is None or response.status < 200 or response.status > 299:
                     raise RSConnectException(
                         "Received an unexpected response from %s (calling %s): %s %s"
                         % (
@@ -2254,6 +2254,10 @@ def should_deploy_as_draft(self, draft: bool, no_verify: bool) -> bool:
         With ``--no-verify`` we activate immediately.
         """
         if draft:
+            if not isinstance(self.client, RSConnectClient):
+                # Neither Connect Cloud nor shinyapps.io has a draft step at any version,
+                # so the version below would send the reader looking for one.
+                raise RSConnectException("Deploying as a draft is only supported by Posit Connect.")
             if not self.supports_verify_before_activate:
                 # We can't honor --draft without the activate field: silently activating
                 # would be the opposite of what the user asked for, so fail loudly.
@@ -3196,11 +3200,10 @@ def update_content(
     ) -> ConnectCloudContent:
         """Update content, optionally minting a fresh revision to upload into.
 
-        `content_type` and `primary_file` are always sent alongside `app_mode`:
-        the API only recomputes `app_mode` when one of them is present in the
-        override set, and the stored content type would otherwise survive a
-        redeploy that changes what kind of content this is (--app-id pointing at
-        content of another type).
+        `revision_overrides` is a partial set: any field left out keeps the value
+        already stored on the content. All three go every time so a redeploy that
+        changes what kind of content this is (--app-id pointing at content of
+        another type) does not keep the old content type.
 
         `secrets` replaces the content's whole set, and `title` overwrites the
         stored one, so both are omitted from the request entirely when None: a

rsconnect/certificates.py

@@ -23,13 +23,8 @@ def read_certificate_file(location: str) -> str | bytes:
 
     path = Path(location)
 
-    # Readability is judged before the suffix so a bad path reports as the real
-    # problem: the file type of a file that does not exist is beside the point.
-    # Both are operational errors: the CLI no longer checks existence at parse
-    # time (the certificate only applies once the target is known), so this is
-    # where a bad path surfaces. is_file() sits inside the handler because it
-    # raises OSError itself when the path's metadata cannot be read, e.g.
-    # through a permission-denied directory.
+    # is_file() is inside the try because it raises OSError itself when the path's
+    # metadata cannot be read, e.g. through a permission-denied directory.
     try:
         if not path.is_file():
             raise RSConnectException("The certificate file %s could not be read: no such file." % location)

rsconnect/metadata.py

@@ -245,26 +245,44 @@ def save_to(self, path: str, data: bytes, open: Callable[..., BufferedWriter] =
             f.write(data)
         self._real_path = path
 
+    def _already_holds(self, path: str, data: bytes) -> bool:
+        """
+        Whether this store was loaded from `path` and that file already holds `data`.
+
+        Restricted to the path we read so a store loaded from its secondary
+        location still migrates to the primary one on the next save.
+        """
+        if self._real_path != path or not exists(path):
+            return False
+        try:
+            with open(path, "rb") as f:
+                return f.read() == data
+        except OSError:
+            return False
+
     # noinspection PyShadowingBuiltins
     def save(self, open: Callable[..., BufferedWriter] = open):
         """
         Save our data to a file.
 
         The app directory is tried first. If that fails,
         then we write to the global config location.
+
+        A save that would not change the file is skipped.
         """
         data = json.dumps(self._data, indent=4).encode("utf-8")
-        try:
-            makedirs(self._primary_path)
-            self.save_to(self._primary_path, data, open)
-        except OSError:
-            if not self._secondary_path:
-                raise
-            makedirs(self._secondary_path)
-            self.save_to(self._secondary_path, data, open)
-
-        if self._chmod and self._real_path is not None:
-            os.chmod(self._real_path, 0o600)
+        if not self._already_holds(self._primary_path, data):
+            try:
+                makedirs(self._primary_path)
+                self.save_to(self._primary_path, data, open)
+            except OSError:
+                if not self._secondary_path:
+                    raise
+                makedirs(self._secondary_path)
+                self.save_to(self._secondary_path, data, open)
+
+            if self._chmod and self._real_path is not None:
+                os.chmod(self._real_path, 0o600)
 
 
 class ServerDataDict(TypedDict):

rsconnect/validation.py

@@ -60,6 +60,38 @@ def _get_present_options(
     return result
 
 
+# Deploy options that configure Posit Connect features Connect Cloud does not
+# have.
+_CONNECT_ONLY_DEPLOY_OPTIONS: dict[str, str] = {
+    "image": "-I/--image",
+    "disable_env_management": "--disable-env-management",
+    "env_management_py": "--disable-env-management-py",
+    "env_management_r": "--disable-env-management-r",
+    "draft": "--draft",
+    "metadata": "--metadata",
+}
+
+
+def _typed_connect_only_deploy_options(ctx: Optional[click.Context]) -> list[str]:
+    """The Connect-only deploy options this command line passed.
+
+    Judged by parameter source rather than by value: --disable-env-management-py
+    inverts to False when given, and the --disable-env-management shorthand fills
+    in the per-language parameters without being their source.
+
+    Only options count. `environment add` takes a positional IMAGE argument, whose
+    parameter is also named `image` and is not this option.
+    """
+    if ctx is None:
+        return []
+    option_names = {param.name for param in ctx.command.params if isinstance(param, click.Option)}
+    return [
+        label
+        for name, label in _CONNECT_ONLY_DEPLOY_OPTIONS.items()
+        if name in option_names and get_parameter_source_name_from_ctx(name, ctx) == "COMMANDLINE"
+    ]
+
+
 def validate_connect_cloud_incompatible_options(
     ctx: Optional[click.Context],
     api_key: Optional[str],
@@ -69,12 +101,12 @@ def validate_connect_cloud_incompatible_options(
 ):
     """Reject options that have no meaning on Posit Connect Cloud.
 
-    validate_connection_options performs the same checks, but only when Connect
-    Cloud is selected by flag or URL; a saved nickname or default server is only
-    identified as Connect Cloud after store resolution, so the executor calls
-    this afterwards. Environment-sourced values are ignored for the same reason
-    as elsewhere: a CONNECT_API_KEY exported for another target is just the
-    CI environment, not a request to use it here.
+    validate_connection_options repeats the credential and SPCS checks, but only
+    when Connect Cloud is selected by flag or URL; a saved nickname or default
+    server is only identified as Connect Cloud after store resolution, so the
+    executor calls this afterwards. Environment-sourced values are ignored for
+    the same reason as elsewhere: a CONNECT_API_KEY exported for another target
+    is just the CI environment, not a request to use it here.
     """
     present_connect_options = _get_present_options(
         {"-k/--api-key": api_key, "-i/--insecure": insecure, "-c/--cacert": cacert},
@@ -92,6 +124,12 @@ def validate_connect_cloud_incompatible_options(
     if present_spcs_options:
         raise RSConnectException(
             f"SPCS options ({', '.join(present_spcs_options)}) may not be passed \
+alongside Posit Connect Cloud. See command help for further details."
+        )
+    connect_only_deploy_options = _typed_connect_only_deploy_options(ctx)
+    if connect_only_deploy_options:
+        raise RSConnectException(
+            f"Posit Connect options ({', '.join(connect_only_deploy_options)}) may not be passed \
 alongside Posit Connect Cloud. See command help for further details."
         )
 

tests/test_connect_cloud.py

@@ -30,12 +30,14 @@
 from rsconnect.metadata import AppStore, ServerData, ServerStore
 from rsconnect.models import AppModes
 from rsconnect.oauth import InvalidClientError, InvalidGrantError
+from rsconnect import validation
 from rsconnect.validation import validate_connection_options
 
 from .utils import failing_keyring
 
 ENV = ParameterSource.ENVIRONMENT
 TYPED = ParameterSource.COMMANDLINE
+DEFAULT = ParameterSource.DEFAULT
 
 
 class TestConnectCloudEnvironments(unittest.TestCase):
@@ -337,8 +339,13 @@ def _json_body(request):
 
 
 def _ctx(**sources: ParameterSource) -> click.Context:
-    """A click context recording where each named parameter's value came from."""
-    ctx = click.Context(click.Command("deploy"))
+    """A click context recording where each named parameter's value came from.
+
+    Each name is declared as an option, since validation distinguishes options
+    from same-named arguments.
+    """
+    params: list[click.Parameter] = [click.Option(["--%s" % param.replace("_", "-")]) for param in sources]
+    ctx = click.Context(click.Command("deploy", params=params))
     for param, source in sources.items():
         ctx.set_parameter_source(param, source)  # pyright: ignore[reportAttributeAccessIssue]
     return ctx
@@ -665,9 +672,9 @@ def _update_content(self, **kwargs):
         return _json_body(httpretty.last_request())
 
     @httpretty.activate(verbose=True, allow_net_connect=False)
-    def test_update_content_sends_primary_file_with_app_mode(self):
-        # The API only recomputes app_mode when content_type or primary_file is
-        # in the override set; sending app_mode alone would persist it verbatim.
+    def test_update_content_sends_the_whole_revision_override_set(self):
+        # Omitted overrides keep the content's stored value, so all three go every
+        # time or a redeploy of a different kind of content keeps the old ones.
         body = self._update_content()
         self.assertEqual(
             body["revision_overrides"],
@@ -2985,6 +2992,73 @@ def test_remove_by_url_with_a_single_entry_removes_it(self):
         self.assertEqual(store.get_all_servers(), [])
 
 
+class TestConnectOnlyDeployOptions(unittest.TestCase):
+    """Deploy options that configure Posit Connect features Connect Cloud does not
+    have. Connect Cloud ignores them, so they are rejected rather than accepted and
+    dropped."""
+
+    OPTIONS = {
+        "image": "-I/--image",
+        "disable_env_management": "--disable-env-management",
+        "env_management_py": "--disable-env-management-py",
+        "env_management_r": "--disable-env-management-r",
+        "draft": "--draft",
+        "metadata": "--metadata",
+    }
+
+    def test_each_is_rejected_with_the_flag(self):
+        for param, label in self.OPTIONS.items():
+            with self.subTest(param):
+                with self.assertRaises(RSConnectException) as context:
+                    _setup_remote_server(ctx=_ctx(**{param: TYPED}), account_name="acme", use_connect_cloud=True)
+                message = str(context.exception)
+                self.assertIn(label, message)
+                self.assertIn("may not be passed alongside Posit Connect Cloud", message)
+
+    def test_each_is_rejected_for_a_saved_cloud_nickname(self):
+        # A nickname is only known to name a Connect Cloud credential after the
+        # store lookup, which is why this is checked in the executor.
+        for param, label in self.OPTIONS.items():
+            with self.subTest(param):
+                with self.assertRaises(RSConnectException) as context:
+                    _setup_remote_server(
+                        ctx=_ctx(**{param: TYPED}), resolve=_cloud_entry(connect_cloud_access_token="at"), name="cloud"
+                    )
+                self.assertIn(label, str(context.exception))
+
+    def test_defaulted_options_are_accepted(self):
+        # --disable-env-management-py inverts to False when given and the shorthand
+        # sets the same parameters without being their source, so what the user
+        # typed can only be read from the parameter source.
+        _cloud_server(ctx=_ctx(image=DEFAULT, env_management_py=DEFAULT), account_name="acme", use_connect_cloud=True)
+
+    def test_a_connect_target_still_accepts_them(self):
+        store = ServerStore(base_dir=tempfile.mkdtemp())
+        store.set("prod", "https://connect.example.com", api_key="key")
+        executor = _setup_remote_server(ctx=_ctx(image=TYPED), store=store, name="prod")
+        self.assertIsInstance(executor.remote_server, api.RSConnectServer)
+
+    def test_draft_at_the_deploy_step_does_not_cite_a_connect_version(self):
+        # The CLI rejects --draft before this, but a programmatic caller has no
+        # click context to judge, and a Connect version says nothing to a target
+        # with no draft step.
+        executor = RSConnectExecutor.__new__(RSConnectExecutor)
+        executor.client = mock.Mock(spec=ConnectCloudClient)
+        with self.assertRaises(RSConnectException) as context:
+            executor.should_deploy_as_draft(draft=True, no_verify=False)
+        message = str(context.exception)
+        self.assertIn("only supported by Posit Connect", message)
+        self.assertNotIn("2025.06.0", message)
+
+    def test_a_same_named_argument_is_not_one_of_these_options(self):
+        # `environment add` takes a positional IMAGE, so a Connect Cloud nickname
+        # must reach that command's own "requires a Posit Connect server" error
+        # rather than be told it passed -I/--image.
+        command = cli.commands["environment"].commands["add"]
+        with command.make_context("add", ["my-image:1.0", "-n", "cloud"]) as ctx:
+            self.assertEqual(validation._typed_connect_only_deploy_options(ctx), [])
+
+
 class TestConnectCloudFindsSavedCredentialsByUrl(unittest.TestCase):
     """`--connect-cloud` and `-s connect.posit.cloud` must find a saved server.
 

tests/test_metadata.py

@@ -1,3 +1,4 @@
+import os
 import shutil
 import tempfile
 from os.path import exists, join
@@ -253,6 +254,31 @@ def test_remove_default_clears_it(self):
         self.server_store.remove_by_name("foo")
         self.assertIsNone(self.server_store.get_default())
 
+    def test_save_skips_rewrite_when_file_unchanged(self):
+        def fail_open(path_to_open, mode, *args, **kw):
+            self.fail("rewrote %s when nothing had changed" % path_to_open)
+
+        self.server_store.save(fail_open)
+
+    def test_save_rewrites_when_data_changed(self):
+        writes = []
+
+        def recording_open(path_to_open, mode, *args, **kw):
+            writes.append(path_to_open)
+            return open(path_to_open, mode, *args, **kw)
+
+        del self.server_store._data["foo"]
+        self.server_store.save(recording_open)
+
+        self.assertEqual(writes, [self.server_store_path])
+        self.assertIsNone(ServerStore(base_dir=self.tempDir).get_by_name("foo"))
+
+    def test_save_rewrites_when_file_removed(self):
+        os.remove(self.server_store_path)
+        self.server_store.save()
+
+        self.assertEqual(len(ServerStore(base_dir=self.tempDir).get_all_servers()), 5)
+
 
 class TestAppMetadata(TestCase):
     def setUp(self):