Client API v2 CLI client: make discriminator subcommand optional by michalvavrik · Pull Request #48018 · keycloak/keycloak

michalvavrik · 2026-04-13T18:25:59Z

Closes: Client API v2 CLI client: make discriminator subcommand optional #47463

Before this PR, when creating / updating / patching clients you had to specify protocol discriminator to distinguish between openid-connect and saml client protocols. However that is not aligned with what we do for list or get commands where you could omit the discriminator sub-command like this:

kcadm.sh client list

For that reason, we now allow to omit the discriminator when you create / update / patch the client from a file like this:

kcadm.sh client patch -f my-patch.json
kcadm.sh client update -f my-update.json
kcadm.sh client create -f my-create.json

However, I did not allow to do this without file (e.g. kcadm.sh client create --client-id test-client) because we would hit potential name conflicts specific for OIDC / SAML and we would also allow users to mix combinations that are not allowed (like options only specific to OIDC and options only specific to SAML). It would be confusing, if users will require it, we can follow-up in the future.

shawkins · 2026-04-14T15:28:20Z

However, I did not allow to do this without file (e.g. kcadm.sh client create --client-id test-client)

This seems good as the descriminator is required.

The new behavior seems good. The only thing that may not be clear is combining -f with fields:

./kcadm.sh --v2 client patch -f my-patch.json saml broker

The -f is effectively ignored.

michalvavrik · 2026-04-14T15:56:18Z

However, I did not allow to do this without file (e.g. kcadm.sh client create --client-id test-client)

This seems good as the descriminator is required.

The new behavior seems good. The only thing that may not be clear is combining -f with fields:

./kcadm.sh --v2 client patch -f my-patch.json saml broker

The -f is effectively ignored.

Damn, I should have had a test for it. I didn't think of it, thank you.

So I think we can simply support ./kcadm.sh --v2 client patch -f my-patch.json saml broker == ./kcadm.sh --v2 client patch saml broker -f my-patch.json. Alternatively, it could be a validation failure, but I'd like to be lenient as users don't care how it works under the hood.

michalvavrik · 2026-04-14T16:47:36Z

Thanks again @shawkins , I have fixed it and added test coverage to make sure it works as expected now.

Pepo48

@michalvavrik thanks for the PR. Not sure if my concern is right, but it feels like skipIdPositional now controls 3 unrelated things at once - whether a user must pass -f, whether to show positional and whether to add -f option group.

This somehow works with the variant parent command, but I'd say it's more like a coincidence. Not sure about my hypothetical scenario, but what about a future variant, e.g. create needs no body? In other words, what if I want to skip but not require -f?

Regardless, whether the example makes sense, I would not keep it coupled this way - it makes the name skipIdPositional is misleading at least ,when it also means "require file".

Below I tried to suggest a small refactor that eliminates the skipIdPositional parameter and the setFileRequired() setter entirely. Wdyt? I also run the tests, everything seemed to work.

Pepo48 · 2026-04-14T16:27:01Z

-
-        CommandLine parentCli = new CommandLine(parentSpec);
-        groupCommand.setSpec(parentSpec);
+        CommandLine parentCli = buildLeafCommand(cmd, null, null, true);


Suggested change

CommandLine parentCli = buildLeafCommand(cmd, null, null, true);

CommandLine parentCli = buildLeafCommand(cmd, null, null);

Pepo48 · 2026-04-14T16:28:30Z

@@ -72,7 +65,14 @@ private static CommandLine buildVariantParentCommand(CommandDescriptor cmd) {

    private static CommandLine buildLeafCommand(CommandDescriptor cmd,
            List<OptionDescriptor> options, VariantDescriptor variant) {
+        return buildLeafCommand(cmd, options, variant, false);
+    }
+


Suggested change

boolean isVariantParent = variant == null

&& cmd.getVariants() != null && !cmd.getVariants().isEmpty();

done, thanks

(I did bit refactoring, but essentially it is your suggestion in there)

Pepo48 · 2026-04-14T16:30:08Z

@@ -87,7 +87,7 @@ private static CommandLine buildLeafCommand(CommandDescriptor cmd,
            addOutputGroup(spec);
        }

-        if (cmd.isRequiresId()) {
+        if (!skipIdPositional && cmd.isRequiresId()) {


Suggested change

if (!skipIdPositional && cmd.isRequiresId()) {

if (!isVariantParent && cmd.isRequiresId()) {

Pepo48 · 2026-04-14T16:30:24Z

@@ -97,21 +97,20 @@ private static CommandLine buildLeafCommand(CommandDescriptor cmd,
                    .build());
        }

-        if (options != null && !options.isEmpty()) {
+        boolean hasFieldOptions = options != null && !options.isEmpty();
+        if (hasFieldOptions || skipIdPositional) {


Suggested change

if (hasFieldOptions || skipIdPositional) {

if (hasFieldOptions || isVariantParent) {

Pepo48 · 2026-04-14T16:31:33Z

    @Spec CommandSpec spec;
    private final CommandDescriptor descriptor;
    private final VariantDescriptor variant;
+    private boolean fileRequired;


Suggested change

private boolean fileRequired;

Pepo48 · 2026-04-14T16:32:57Z

+    void setFileRequired(boolean fileRequired) {
+        this.fileRequired = fileRequired;


Suggested change

void setFileRequired(boolean fileRequired) {

this.fileRequired = fileRequired;

private boolean isVariantParent() {

return variant == null

&& descriptor.getVariants() != null && !descriptor.getVariants().isEmpty();

Pepo48 · 2026-04-14T16:33:52Z

            String body = buildRequestBody();
+            if (body == null && fileRequired) {


Suggested change

if (body == null && fileRequired) {

if (body == null && isVariantParent()) {

Pepo48 · 2026-04-14T17:13:39Z

+            throw new RuntimeException("Cannot extract '" + descriptor.getResourceName()
+                    + "' resource ID from file. Use a subcommand that accepts <id> instead.");
+        }
+        Objects.requireNonNull(body, "Missing required file (-f/--file) with '" + idField + "' field");


Suggested change

Objects.requireNonNull(body, "Missing required file (-f/--file) with '" + idField + "' field");

I guess this is a dead code - fileRequired at line 161-164 always catches body==null for variant parents and PicoCLI enforces for leaf commands.

I guess this is a dead code - fileRequired at line 161-164 always catches body==null for variant parents and PicoCLI enforces for leaf commands.

good catch

Pepo48 · 2026-04-14T17:15:15Z

@@ -11,6 +11,7 @@
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;


Suggested change

import java.util.Objects;

Pepo48

As for the tests, I think we could cover two more scenarios - for client patch -f bad.json (malformed json) and for client patch -f {"clientId": ""} (blank value).

Also, testClientCreateSamlFromFile doesn't call extractId to verify the created client can be fetched, while testClientCreateOidcFromFile counterpart does that, so I would add it to keep the test symmetry.

michalvavrik · 2026-04-14T17:38:58Z

As for the tests, I think we could cover two more scenarios - for client patch -f bad.json (malformed json) and for client patch -f {"clientId": ""} (blank value).

certainly, I'll do it. Thanks.

Also, testClientCreateSamlFromFile doesn't call extractId to verify the created client can be fetched, while testClientCreateOidcFromFile counterpart does that, so I would add it to keep the test symmetry.

That actually was intentional, I thought we should know that it works as well. I'll make testClientCreateSamlFromFile and testClientCreateOidcFromFile symmetric and add one more test to cover what is in testClientCreateSamlFromFile to be sure.

michalvavrik · 2026-04-14T17:43:18Z

sorry, I misinterpreted your testClientCreateSamlFromFile comment, should had look instead of answering from top of my head. sure, agreed

* Closes: keycloak#47463 Signed-off-by: Michal Vavřík <michal.vavrik@aol.com>

Pepo48

LGTM now, thanks Michal!

Copilot

Pull request overview

This PR updates the Keycloak Admin CLI v2 “client” commands to allow omitting the protocol discriminator subcommand (oidc/saml) when creating/updating/patching from a JSON file, aligning behavior with list/get which already work without a discriminator.

Changes:

Make client create|update|patch -f/--file ... work at the discriminator “parent” command level (no oidc/saml required), including extracting the required path <id> from file content when needed.
Improve CLI UX around this mode: help output and shell completion now show -f/--file on the variant parent and hide protocol-specific field options there.
Add integration/unit tests covering success paths and failure modes (missing file, duplicate -f, missing/blank ID in file, malformed JSON).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
`rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/cli/v2/KcAdmV2ClientCLITest.java`	Adds end-to-end integration tests for create/update/patch without discriminator when `-f/--file` is used, plus negative cases.
`integration/client-cli/admin-cli/src/test/java/org/keycloak/client/admin/cli/commands/v2/KcAdmV2HelpTest.java`	Verifies variant-parent help includes `-f/--file`, omits `<id>` and protocol field options, and still shows connection options.
`integration/client-cli/admin-cli/src/test/java/org/keycloak/client/admin/cli/commands/v2/KcAdmV2CompleterTest.java`	Ensures autocomplete suggests `-f` at the variant parent and does not suggest protocol-specific field options there.
`integration/client-cli/admin-cli/src/main/java/org/keycloak/client/admin/cli/v2/KcAdmV2RequestExecutor.java`	Implements parent/leaf `-f` resolution, rejects missing file at variant parent, and extracts `<id>` from JSON when positional ID is absent.
`integration/client-cli/admin-cli/src/main/java/org/keycloak/client/admin/cli/v2/KcAdmV2CommandDescriptor.java`	Adds `hasVariants()` helper to simplify variant detection.
`integration/client-cli/admin-cli/src/main/java/org/keycloak/client/admin/cli/v2/KcAdmV2CommandBuilder.java`	Builds executable variant-parent commands with `-f/--file` option and suppresses `<id>` positional for the parent.

shawkins · 2026-04-15T19:10:57Z

Thank you @michalvavrik, merged based upon my, @Pepo48, and copilot's reviews

michalvavrik requested review from a team as code owners April 13, 2026 18:26

keycloak-github-bot Bot added team/cloud-native labels Apr 13, 2026

michalvavrik requested a review from shawkins April 13, 2026 19:33

michalvavrik force-pushed the feature/issues/47463 branch 2 times, most recently from 22c30c1 to 8c48ffe Compare April 14, 2026 16:46

Pepo48 reviewed Apr 14, 2026

View reviewed changes

Pepo48 requested changes Apr 14, 2026

View reviewed changes

feat(admin-api-v2): make CLI discriminator subcommand optional

3ec5368

* Closes: keycloak#47463 Signed-off-by: Michal Vavřík <michal.vavrik@aol.com>

michalvavrik force-pushed the feature/issues/47463 branch from 8c48ffe to 3ec5368 Compare April 14, 2026 18:27

michalvavrik requested a review from Pepo48 April 14, 2026 18:36

Pepo48 approved these changes Apr 15, 2026

View reviewed changes

shawkins requested a review from Copilot April 15, 2026 15:42

Copilot started reviewing on behalf of shawkins April 15, 2026 15:44 View session

Copilot AI reviewed Apr 15, 2026

View reviewed changes

shawkins merged commit f303cc8 into keycloak:main Apr 15, 2026
89 checks passed

michalvavrik deleted the feature/issues/47463 branch April 15, 2026 20:22

stianst mentioned this pull request Jun 1, 2026

js deps uuid 26.6 #49539

Closed

	CommandLine parentCli = buildLeafCommand(cmd, null, null, true);
	CommandLine parentCli = buildLeafCommand(cmd, null, null);



	boolean isVariantParent = variant == null
	&& cmd.getVariants() != null && !cmd.getVariants().isEmpty();

	if (!skipIdPositional && cmd.isRequiresId()) {
	if (!isVariantParent && cmd.isRequiresId()) {

	if (hasFieldOptions \|\| skipIdPositional) {
	if (hasFieldOptions \|\| isVariantParent) {

		void setFileRequired(boolean fileRequired) {
		this.fileRequired = fileRequired;

-    void setFileRequired(boolean fileRequired) {
-        this.fileRequired = fileRequired;
+    private boolean isVariantParent() {
+        return variant == null
+                && descriptor.getVariants() != null && !descriptor.getVariants().isEmpty();

		String body = buildRequestBody();
		if (body == null && fileRequired) {

	if (body == null && fileRequired) {
	if (body == null && isVariantParent()) {

Conversation

michalvavrik commented Apr 13, 2026

Uh oh!

shawkins commented Apr 14, 2026

Uh oh!

michalvavrik commented Apr 14, 2026

Uh oh!

michalvavrik commented Apr 14, 2026

Uh oh!

Pepo48 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pepo48 left a comment

Choose a reason for hiding this comment

Uh oh!

michalvavrik commented Apr 14, 2026

Uh oh!

michalvavrik commented Apr 14, 2026

Uh oh!

Pepo48 left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

shawkins commented Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants