`ami section` fails with valid `--summary` argument #48

petermr · 2020-07-04T12:10:38Z

ami [...] section

runs correctly without options. and with

ami [...] section --sections

but fails with

ami [...] section --sections ALL --summary figure

see: https://github.com/petermr/ami3/blob/master/src/test/java/org/contentmine/ami/tools/AMISectionToolTest.java

	public void testSectionsSummaryBug() {
		String args;
		args = ""
				+ "-p " + AMIFixtures.TEST_ZIKA10_DIR
				+ " --forcemake"
				+ " section"
				+ " --sections ALL"
			;
		AMI.execute(AMISectionTool.class, args);
		System.err.println("=======end 1 runs OK ========");
		args = ""
				+ "-p " + AMIFixtures.TEST_ZIKA10_DIR
				+ " --forcemake"
				+ " section"
				+ " --sections ALL"
				+ " --summary foo"
			;
		AMI.execute(AMISectionTool.class, args);
		System.err.println("=======end 2 runs OK, detects bad arg 'foo' ========\n"
				+ "Invalid value for option '--summary' at index 0 (<summaryList>): "
				+ "    expected one of [figure, results, supplementary, table] (case-sensitive) but was 'foo'\n" + 
				"Usage: ami section [OPTIONS]\n" + 
				"Try 'ami section --help' for more information.\n"
				+ "===============================" + 
				"");
		args = ""
				+ "-p " + AMIFixtures.TEST_ZIKA10_DIR
				+ " --forcemake"
				+ " section"
				+ " --sections ALL"
				+ " --summary figure"
			;
		AMI.execute(AMISectionTool.class, args);
		System.err.println("=======end 3 fails========\n"
				+ "Expected parameter for option '--summary' but found 'figure'\n" + 
				"Usage: ami section [OPTIONS]\n" + 
				"Try 'ami section --help' for more information.\n" + 
				"==============================");
		
	}

The text was updated successfully, but these errors were encountered:

remkop · 2020-07-04T12:31:50Z

Have you tried removing arity = "0..*" from that option definition?

petermr · 2020-07-04T18:02:47Z

Note - being run programmatically.

Here's the test:

		args = ""
				+ "-p " + AMIFixtures.TEST_ZIKA10_DIR
				+ " --forcemake"
				+ " section"
				+ " --sections ALL"
				+ " --summary foo"
			;
		AMI.execute(AMISectionTool.class, args);
		System.err.println(""
				+ "=======2 runs OK: --summary detects bad arg 'foo' ========\n"
				);
		args = ""
				+ "-p " + AMIFixtures.TEST_ZIKA10_DIR
				+ " --forcemake"
				+ " section"
				+ " --sections ALL"
				+ " --summary figure"
			;
		AMI.execute(AMISectionTool.class, args);
		System.err.println(""
				+ "=======3 fails: Expected parameter for option '--summary' but found 'figure'========\n"
				);

I'll run it with varying Option definitions:

no arity

    @Option(names = {"--sections"},
    		arity = "1..*",
            description = "sections to extract (uses JATSSectionTagger) %n"
            		+ "if none, lists Tagger tags%n"
            		+ "ALL selects all tags in Tagger%n"
            		+ "AUTO creates hierchical tree based on JATS and heuristics (default)%n,"
            		)
    private List<SectionTag> sectionTagList = 
                        new ArrayList<>(Arrays.asList(new SectionTag[]{SectionTag.AUTO}));

    @Option(names = {"--sectiontype"},
    		arity = "1",
            description = "Type of section (XML or HTML) default XML. Probably only used in development")
    private SectionType sectionType = SectionType.XML;

    @Option(names = {"--summary"},
            description = "create summary files for sections (${COMPLETION-CANDIDATES})")
    private List<SummaryType> summaryList = new ArrayList<>();

gives

Invalid value for option '--summary' (<summaryList>): expected one of [figure, results, supplementary, table] (case-sensitive) but was 'foo'
Usage: ami section [OPTIONS]
Try 'ami section --help' for more information.
=======2 runs OK: --summary detects bad arg 'foo' ========

Expected parameter for option '--summary' but found 'figure'
Usage: ami section [OPTIONS]
Try 'ami section --help' for more information.
=======3 fails: Expected parameter for option '--summary' but found 'figure'========

arity 1..*

    @Option(names = {"--sections"},
    		arity = "1..*",
            description = "sections to extract (uses JATSSectionTagger) %n"
            		+ "if none, lists Tagger tags%n"
            		+ "ALL selects all tags in Tagger%n"
            		+ "AUTO creates hierchical tree based on JATS and heuristics (default)%n,"
            		)
    private List<SectionTag> sectionTagList = 
                        new ArrayList<>(Arrays.asList(new SectionTag[]{SectionTag.AUTO}));

    @Option(names = {"--sectiontype"},
    		arity = "1",
            description = "Type of section (XML or HTML) default XML. Probably only used in development")
    private SectionType sectionType = SectionType.XML;

    @Option(names = {"--summary"},
    		arity = "1..*",
            description = "create summary files for sections (${COMPLETION-CANDIDATES})")
    private List<SummaryType> summaryList = new ArrayList<>();

gives

Invalid value for option '--summary' at index 0 (<summaryList>): expected one of [figure, results, supplementary, table] (case-sensitive) but was 'foo'
Usage: ami section [OPTIONS]
Try 'ami section --help' for more information.
=======2 runs OK: --summary detects bad arg 'foo' ========

Expected parameter for option '--summary' but found 'figure'
Usage: ami section [OPTIONS]
Try 'ami section --help' for more information.
=======3 fails: Expected parameter for option '--summary' but found 'figure'========

remkop · 2020-07-04T23:06:33Z

I found the cause:
figure is also the name of the AMIFigureTool, which is a sibling command of section.

Picocli interprets figure as a new command (since we configured AMI with subcommandsRepeatable = true); and thus concludes that the --summary option was missing a mandatory parameter.

We need to disambiguate this somehow.
One idea is to rename either the name for AMIFigureTool, or the AMISectionTool.SummaryType enum value.

An alternative is to require end users to quote the value for the --summary option:

-p src\test\resources\org\contentmine\ami\zika10 --forcemake section --sections ALL --summary "figure"

(The latter means a minor code change in AMI:

Index: src/main/java/org/contentmine/ami/tools/AMI.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/main/java/org/contentmine/ami/tools/AMI.java	(date 1593903767568)
+++ src/main/java/org/contentmine/ami/tools/AMI.java	(date 1593903767568)
@@ -165,6 +165,7 @@
 
 	private static CommandLine createCommandLine() {
 		CommandLine cmd = new CommandLine(new AMI());
+		cmd.setTrimQuotes(true);
 		cmd.setParameterExceptionHandler(new ShortErrorMessageHandler());
 		cmd.setExecutionStrategy(AMI::enhancedLoggingExecutionStrategy);
 		return cmd;

petermr · 2020-07-05T07:48:30Z

On Sun, Jul 5, 2020 at 12:06 AM Remko Popma ***@***.***> wrote: I found the cause: figure is also the name of the AMIFigureTool, which is a sibling command of section.

WELL DONE! I hope it didn't take too long. It's obvious in retrospect.

Picocli interprets figure as a new command (since we configured AMI with subcommandsRepeatable = true); and thus concludes that the --summary option was missing a mandatory parameter.

Understood

We need to disambiguate this somehow. One idea is to rename either the name for AMIFigureTool, or the AMISectionTool.SummaryType enum value.

The single fix is easy - I will rename the enum value. It's clearly a generic problem and common to any system which uses reserved names. I sometimes us "default" as a name in Java until the compiler refuses.

An alternative is to require end users to quote the value for the --summary option: -p src\test\resources\org\contentmine\ami\zika10 --forcemake section --sections ALL --summary "figure" I don't think we need this. It's best for me to choose non-clashing

keywords. The enums are under my control. In principle we could check the values against commands. Another idea would be syntax for the commands, e.g. case or prefix. I don't like this but it would be safe: ami -p foo c_search ... c_summary Are there other scenarios for clashes?

…

-- Peter Murray-Rust Founder ContentMine.org and Reader Emeritus in Molecular Informatics Dept. Of Chemistry, University of Cambridge, CB2 1EW, UK

remkop · 2020-07-05T08:18:10Z

I haven’t searched if any other enum values could clash with other command names.

I agree that renaming is probably the easiest and most reliable solution.

petermr · 2020-07-05T08:24:56Z

The problem is that it only surfaces when the enum value is used. I think I need a way of validating enum values at regular intervals. Does the use of "=" to identify parameter values avoid this? P.

…

On Sun, Jul 5, 2020 at 9:18 AM Remko Popma ***@***.***> wrote: I haven’t searched if any other enum values could clash with other command names. I agree that renaming is probably the easiest and most reliable solution. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#48 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFTCS7PHWLEBN6PWOWF3SLR2AZM3ANCNFSM4OQL45WA> .

-- Peter Murray-Rust Founder ContentMine.org and Reader Emeritus in Molecular Informatics Dept. Of Chemistry, University of Cambridge, CB2 1EW, UK

remkop · 2020-07-05T09:04:39Z

I haven’t checked but I suspect that putting = between the option name and the value doesn’t help. That is a good point and potentially something to improve in picocli. (Requires some thought.)

petermr · 2020-07-05T13:19:59Z

Also, what happens with uncontrolled strings? e.g. if a project has the same name as a reserved (sub) command: ``` ami -p search search --dictionary search ``` where I have a project `search` (sloppy, easy to do) and a dictionary called `search` (maps to `search.xml`). It's not easy building parsers - I know. I use/d ANTLR which is powerful but wasn't easy to load - but I think it's improved over the years. Is there a formal language specification for picocli? Could one generate a parser from the Commands and Options. Maybe a Parser generator would show the conflicts. P.

…

On Sun, Jul 5, 2020 at 10:04 AM Remko Popma ***@***.***> wrote: I haven’t checked but I suspect that putting = between the option name and the value doesn’t help. That is a good point and potentially something to improve in picocli. (Requires some thought.) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#48 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFTCS64A23AQYGV432ZL5DR2A63FANCNFSM4OQL45WA> .

-- Peter Murray-Rust Founder ContentMine.org and Reader Emeritus in Molecular Informatics Dept. Of Chemistry, University of Cambridge, CB2 1EW, UK

remkop · 2020-07-06T03:26:03Z

what happens with uncontrolled strings? e.g. if a project has the same name as a reserved (sub) command:
ami -p search search --dictionary search
where I have a project search (sloppy, easy to do) and a dictionary called search (maps to search.xml).

Good point. I had not thought of this.

With picocli 4.3.x, the string "search" will be accepted and assigned to the -p option without any validation.
With picocli 4.4, the above input will fail with error Expected parameter for option '-p' but found 'search'.
(Background: remkop/picocli#1055)

The picocli 4.4 release notes mention the related case of using option names as option value, for example, what if you have a project that was named --ctree for some reason. The recommendation is to quote such values. This would also work for your example:

ami -p "search" search --dictionary "search"

However, application authors may want to allow values that match subcommands (or even values that match option names) as option parameters... I am beginning to think the picocli parser is now too strict. I will think about how to address this in a future version of picocli.

It's not easy building parsers - I know. I use/d ANTLR which is powerful but wasn't easy to load - but I think it's improved over the years. Is there a formal language specification for picocli? Could one generate a parser from the Commands and Options. Maybe a Parser generator would show the conflicts.

I am familiar with ANTLR. I don't want to bring in ANTLR as a dependency, but I see your point about formalizing how the picocli parser works. There is no formal language specification, because each picocli application has a unique grammar, defined in their app with picocli annotations. That said, it may be worth formalizing the grammar for the composing elements.

At any rate, I need to think about how to give application flexibility in how to treat ambiguous parameter values.
Similarly for non-enum values you mentioned in your previous comment. This needs more thought.

remkop · 2020-07-24T22:38:54Z

Closing since the original problem has been resolved and any follow-up work is in the picocli project.

remkop mentioned this issue Jul 7, 2020

Options disallow values that match subcommand name remkop/picocli#1125

Closed

remkop closed this as completed Jul 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`ami section` fails with valid `--summary` argument #48

`ami section` fails with valid `--summary` argument #48

petermr commented Jul 4, 2020

remkop commented Jul 4, 2020

petermr commented Jul 4, 2020 •

edited

Loading

remkop commented Jul 4, 2020

petermr commented Jul 5, 2020 via email

remkop commented Jul 5, 2020

petermr commented Jul 5, 2020 via email

remkop commented Jul 5, 2020

petermr commented Jul 5, 2020 via email

remkop commented Jul 6, 2020

remkop commented Jul 24, 2020

ami section fails with valid --summary argument #48

ami section fails with valid --summary argument #48

Comments

petermr commented Jul 4, 2020

remkop commented Jul 4, 2020

petermr commented Jul 4, 2020 • edited Loading

no arity

arity 1..*

remkop commented Jul 4, 2020

petermr commented Jul 5, 2020 via email

remkop commented Jul 5, 2020

petermr commented Jul 5, 2020 via email

remkop commented Jul 5, 2020

petermr commented Jul 5, 2020 via email

remkop commented Jul 6, 2020

remkop commented Jul 24, 2020

`ami section` fails with valid `--summary` argument #48

`ami section` fails with valid `--summary` argument #48

petermr commented Jul 4, 2020 •

edited

Loading