Fixed over-quoted examples for string definitions #5778

JFCote · 2017-06-05T17:58:24Z

PR checklist

Read the contribution guidelines.
Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)
Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

This is quick fix for #5460

I don't know if it's a "good" way to deal with this but my understanding is that a "string" model is the exception to the rule because the example is not "json" but a simple string, so there is no need to double quote it.

I didn't ran the shell/batch script because I didn't know if there were a quick way to run them all at once... @wing328 , if the fix is accepted, can you run the appropriate script? Thanks!

wing328 · 2017-06-10T00:50:08Z

@JFCote thanks for the fix. Am I correct in saying that the fix won't work with examples that contain double quote?

ePaul · 2017-06-10T17:46:25Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/InlineModelResolver.java


+    private void fixStringModel(ModelImpl m) {
+        if (m.getType() != null && m.getType().equals("string") && m.getExample() != null) {
+            m.setExample(m.getExample().toString().replace("\"", ""));


As @wing328 mentioned, this removes all the " from the example string, which likely is not what we want.
Could we restrict this to the first/last character?

Unfortunately, Swagger-models (or at least the classes/interfaces in use here) have no javadocs, so it is not really clear what should be in this example property.

Yes you are right, I didn't think about this possible scenario. I will submit something today to fix this. Thanks for the review @wing328 and @ePaul

JFCote · 2017-06-14T12:12:30Z

Will take a little bit more time for my fix, I'm pretty busy right now! Just wanted to let you know I did not forget about this

wing328 · 2017-06-14T13:14:53Z

@JFCote I'll take a look tomorrow to see if there're other ways to address it

…ble quote in the middle

JFCote · 2017-06-15T13:10:45Z

There you go. It will only remote the double quote if they are at the beginning and the end of the example, when it's a string. Like I said, I did not run all of the sample .sh, I don't know how to do it all at once.

ePaul · 2017-06-15T13:32:46Z

@JFCote you could use bin/run-all-petstore ... it gives a warning telling that it is meant to be used just from CI, but it works (at least for me). (It will give some unrelated changes due to other people not having run it before.)

Do we have a test case in the examples where it formerly was wrong?

ePaul · 2017-06-15T13:34:31Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/InlineModelResolver.java

+            String example = m.getExample().toString();
+            if (example.substring(0, 2).equals("\"") &&
+                    example.substring(example.length() - 2).equals("\"")) {
+                m.setExample(example.substring(2, example.length() - 2));


I think your count is wrong here. The Java string "\"Hello\"" has length 7, not 9, and you need to cut off one character at each end, not two. Though please test this first.

I've test it and it works. And we want to cut "\"", which is 2 chars.

For example:

0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 \ | " | H | E | L | L | O | \ | "

First condition will return \" which is true
Second condition will return 9 - 2 = 7 -> \"
Then we simply remove everything between both which returns HELLO

I've test it and it works :) Let me know if it doesn't work in your case and I'll look into it!

Hmm, maybe I misunderstood what you are doing here ... I thought we have here a pair of " too much, which needs to be removed. (This is what the old replace code was doing, assuming there are no more quotation marks in the string.)
I thought the escaping (" → \") was only done after this step (while outputting).

Anyway, if you want to remove not just a single ", but the combination \", then you'll need to write it "\\\"" as a Java string literal (the single backslash is escaping the ").

Though I'll try it out instead of just using my Java knowledge to discuss things theoretically.

@JFCote I added the example snippet to src/main/resources/petstore.yaml, and regenerated the code (from your branch). The bug still occurs there. Using 1 instead of 2 fixes it.

(Feel free to pick those commits from my fork, or copy just the needed parts.)

You are right... I really don't understand how it worked for me (tested it a couple of times...) but now, it doesn't work.
I will submit your fix in a couple of minutes..

JFCote · 2017-06-15T13:39:04Z

@ePaul : For an example, check #5460
You just have to add this to your list of definitions:

ReturnStatus:
  type: string
  enum:
    - NotReturned
    - Recycled
    - Trashed
    - InTransit
    - InvestigationPending
    - Lost
  description: "Device's return status"
  example: InTransit

…gger-api#5460.

JFCote · 2017-06-19T17:56:08Z

@ePaul : I added you proposed changes to the PR. Thanks for the help and sorry again, I really don't know what happened during my testing!

JFCote · 2017-07-05T18:29:10Z

@ePaul and @wing328 : Is there something blocking this PR to be merged? I have tried to run the bin/run-all-petstore but there is tons of changes not related to my own changes, just because it's not everybody that runs the .sh after each changes. I'm not at ease to commit all of this in the PR unless you ask me :)

Let me know what I can do to help the merge of this PR. Thanks!

ePaul · 2017-07-06T16:01:24Z

@JFCote Looking at the code, I don't see anything blocking this.

I can't build the project at my work computer currently (I'm stuck with an older maven installation), will try it later at home to see which changes actually come from this PR and which are already in master.

wing328 · 2017-07-06T16:17:16Z

@JFCote I'll take a look tomorrow.

ePaul · 2017-07-06T20:16:58Z

I just tried the sample-updating once on master (see #6004), and once again on the merge commit of your branch. Same result (apart from two markdown/txt files containing the build date).

A question would be, could we have some examples so that there is actually a difference?

JFCote · 2017-07-07T12:13:23Z

@ePaul : Should I create a special petstore.yaml for this or modify the main yaml file that every generator use?

wing328 · 2017-07-07T13:23:54Z

@JFCote I think we can add a few to the existing petstore.yaml. I'll try to do it later and run some tests with your PR.

wing328 · 2017-07-07T13:24:42Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/InlineModelResolver.java

        }
    }

+    private void fixStringModel(ModelImpl m) {


@JFCote it would be nice to add a 1-liner or a proper docstring explaining what this function does.

… called PetStatus that test this scenario. Update sample only for JavaPlayFramework generator

JFCote · 2017-07-07T14:48:34Z

Ok, I have added the comment + example in the petstore.yaml file.
I have only updated the sample for the Play Framework. Please feel free to run the bin/run-all-petstore file later to update all the other generators. If you want to see the fix itself in action, using the new petstore.yaml, remove the content of the fixStringModel function and you will see the double quoting reappear proving that the fix works.

ePaul · 2017-07-18T21:37:20Z

I tried updating the other generators, no changes (which were not already changes before). (I guess they don't use the same YAML, or don't use the examples.)

So 👍 from me.

JFCote · 2017-07-19T19:29:20Z

@wing328 Can you merge this when you have time? Thanks!

wing328 · 2017-07-20T16:25:53Z

@JFCote I'll do it tomorrow (sorry for the delay. too busy these days..)

JFCote · 2017-07-20T16:27:56Z

@wing328 No problem man! I completely understand!

wing328 · 2017-07-20T16:58:43Z

modules/swagger-codegen/src/test/resources/2_0/petstore.yaml

        type: string
      message:
        type: string
+  PetStatus:


@JFCote I'll remove this tomorrow add the enum example to https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml instead.

When the Play generator, we should consider using petstore-with-fake-endpoints-models-for-testing.yaml, which covers a lot more edge cases.

wing328 · 2017-07-24T17:21:47Z

Removed "PetStatus" via 943b47d

Fix issue 5460

eefcdca

ePaul reviewed Jun 10, 2017

View reviewed changes

Handling only first and last double quote to support example with dou…

5d4a1fd

…ble quote in the middle

ePaul reviewed Jun 15, 2017

View reviewed changes

ePaul pushed a commit to ePaul/swagger-codegen that referenced this pull request Jun 15, 2017

Add test case with string model with example for swagger-api#5778/swa…

947303b

…gger-api#5460.

Fix proposed by @ePaul

341fa15

wing328 reviewed Jul 7, 2017

View reviewed changes

Add comments to explain de fixStringModel function. Add an enum model…

8ee0e12

… called PetStatus that test this scenario. Update sample only for JavaPlayFramework generator

wing328 reviewed Jul 20, 2017

View reviewed changes

Merge branch 'master' into fix-5460

8809792

wing328 added Server: Java Issue: Bug labels Jul 24, 2017

wing328 added the Feature: Enum label Jul 24, 2017

wing328 added this to the v2.3.0 milestone Jul 24, 2017

wing328 merged commit 79e10c4 into swagger-api:master Jul 24, 2017

wing328 changed the title ~~Fix for issue #5460~~ Fixed over-quoted examples for string definitions Jul 26, 2017

JFCote deleted the fix-5460 branch August 7, 2017 16:28

joeljons mentioned this pull request Nov 13, 2017

Over quoted string example swagger-api/swagger-parser#579

Merged

Fixed over-quoted examples for string definitions #5778

Fixed over-quoted examples for string definitions #5778

Uh oh!

Conversation

JFCote commented Jun 5, 2017

PR checklist

Description of the PR

Uh oh!

wing328 commented Jun 10, 2017

Uh oh!

ePaul Jun 10, 2017

Choose a reason for hiding this comment

Uh oh!

JFCote Jun 12, 2017

Choose a reason for hiding this comment

Uh oh!

JFCote commented Jun 14, 2017

Uh oh!

wing328 commented Jun 14, 2017

Uh oh!

JFCote commented Jun 15, 2017

Uh oh!

ePaul commented Jun 15, 2017

Uh oh!

ePaul Jun 15, 2017

Choose a reason for hiding this comment

Uh oh!

JFCote Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ePaul Jun 15, 2017

Choose a reason for hiding this comment

Uh oh!

ePaul Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JFCote Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

JFCote commented Jun 15, 2017

Uh oh!

JFCote commented Jun 19, 2017

Uh oh!

JFCote commented Jul 5, 2017

Uh oh!

ePaul commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wing328 commented Jul 6, 2017

Uh oh!

ePaul commented Jul 6, 2017

Uh oh!

JFCote commented Jul 7, 2017

Uh oh!

wing328 commented Jul 7, 2017

Uh oh!

wing328 Jul 7, 2017

Choose a reason for hiding this comment

Uh oh!

JFCote commented Jul 7, 2017

Uh oh!

ePaul commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JFCote commented Jul 19, 2017

Uh oh!

wing328 commented Jul 20, 2017

Uh oh!

JFCote commented Jul 20, 2017

Uh oh!

wing328 Jul 20, 2017

Choose a reason for hiding this comment

Uh oh!

wing328 commented Jul 24, 2017

Uh oh!

Reviewers

Assignees

Labels

JFCote Jun 15, 2017 •

edited

Loading

ePaul Jun 15, 2017 •

edited

Loading

ePaul commented Jul 6, 2017 •

edited

Loading

ePaul commented Jul 18, 2017 •

edited

Loading