Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using 'properties' property name in Model results in it not showing up correctly on the UI models #4228

Closed
saarivirtajCGI opened this issue Feb 15, 2018 · 21 comments · Fixed by swagger-api/swagger-js#1368

Comments

@saarivirtajCGI
Copy link

saarivirtajCGI commented Feb 15, 2018

Q A
Bug or feature request? Bug
Which Swagger/OpenAPI version? 2.0
Which Swagger-UI version? 3.10.0
How did you install Swagger-UI? Downloaded latest source package, copied dist/ files
Which browser & version? Chromium
Which operating system? Windows 10

Demonstration API definition

When you have a property that is named "properties" the UI doesn't seem to respond well to it.

This can be verified by setting a property's name "properties". I've included sample swagger.json files for inspection:

swagger.json with 'properties'
swagger.json with 'properties' renamed to 'xxx'

When the 'properties' is renamed to 'xxx', everything then works as expected.

This visual diff shows the only difference in the spec:
https://i.imgur.com/cGvZOl4.png

Expected Behavior

Should display the models correctly, like this (see 'xxx' / ProductModel):
https://i.imgur.com/uYW1huI.png
https://i.imgur.com/HTViGLA.png

Current Behavior

Apparently because the property is called 'properties', the UI gets confused, and this is what we see:
https://i.imgur.com/IM3oxDw.png
https://i.imgur.com/sc06JKU.png

The property completely disappears from the model, and the type 'Props' is not resolved in the ProductModel, even though it's clearly defined.

Context

Having a property called 'properties' shouldn't break things in my opinion.

@heldersepu
Copy link
Contributor

heldersepu commented Feb 15, 2018

I just added one and I did not find any problem:
http://swashbuckletest.azurewebsites.net/swagger/ui/index#/Company/Company_Post

@saarivirtajCGI
Copy link
Author

@heldersepu
Copy link
Contributor

What I was trying to proof is that is not just using 'properties' property name in Model...
Like my sample shows it worked on that case, There might be more to this issue.

@webron
Copy link
Contributor

webron commented Feb 16, 2018

Huh, that's kinda funny (okay, maybe not for you, @saarivirtajCGI). It looks like it's when there's a $ref for the value...

@hctv19
Copy link

hctv19 commented Mar 19, 2018

Has there been any progress made on this bug, or is there a known workaround for it? This one's biting us as well.

@thePanz
Copy link

thePanz commented Apr 13, 2018

I guess the same issue has been reported here: #3091

@heldersepu
Copy link
Contributor

@hctv19 I think the workaround here is obvious: Don't name your property "properties"

@epet
Copy link

epet commented May 3, 2018

@shockey, thanks for picking this bug up. When do you think this might land on your inner radar circles as we just had a customer report on this issue.

@heldersepu
Copy link
Contributor

@shockey I think the correction is simple, we just need to add it to the freelyNamedPaths:
https://github.com/swagger-api/swagger-js/blob/master/src/specmap/helpers.js

@epet
Copy link

epet commented May 3, 2018

Hey @shockey, if @heldersepu is correct, I'm happy to send that PR?

@heldersepu
Copy link
Contributor

@epet Sure if you have time send a PR, you don't have to ask permission for that...

but make sure to include some unit tests, look at the history of that file I added something not too long ago and I included tests, you can use that as a starting point

@shockey
Copy link
Contributor

shockey commented May 15, 2018

@epet - yeah, by all means, open a PR 😄

@epet
Copy link

epet commented May 17, 2018

This is probably my lack of understanding nuances of the codebase but I put this test in my change and in the current master, it passes in both versions:

    it('should ignore $refs in freely named Swagger nested positions.', function () {
      return mapSpec({
        spec: {
          a: 1234,
          parameters: {
            $ref: '#/a'
          },
          responses: {
            $ref: '#/a'
          },
          definitions: {
            $ref: '#/a'
          },
          securityDefinitions: {
            $ref: '#/a'
          },
          properties: {
            properties: {
              $ref: '#/a'
            }
          },
          foo: {
            properties: {
              $ref: '#/a'
            }
          }
        },
        plugins: [refs],
      }).then((res) => {
        expect(res.spec).toEqual({
          a: 1234,
          parameters: {
            $ref: '#/a'
          },
          responses: {
            $ref: '#/a'
          },
          definitions: {
            $ref: '#/a'
          },
          securityDefinitions: {
            $ref: '#/a'
          },
          properties: {
            properties: {
              $ref: '#/a'
            }
          },
          foo: {
            properties: {
              $ref: '#/a'
            }
          }
        })
      })
    })

I'm not sure this is solving a problem. @heldersepu or @shockey, advice for better tests or validation for this scenario?

@heldersepu
Copy link
Contributor

heldersepu commented May 17, 2018

Bamm. Did the problem got fix?

@epet Maybe your unit-test does not actually test the problem, I remember that the first time around I was not able to reproduce, maybe your are on the same boat.

@heldersepu
Copy link
Contributor

@epet
Copy link

epet commented May 18, 2018

being a managed code developer, I'm ramping up with the javascript environments. I finally got my changes setup in a local branch with the swagger-ui. Adding 'properties' to the freelyNamedPaths does not solve the issue we are seeing. Open to other ideas or I will pass to let someone with more depth in this codebase fix it.

@shockey shockey added this to the May 25, 2018 milestone May 22, 2018
@heldersepu
Copy link
Contributor

heldersepu commented May 28, 2018

@epet I feel your pain!

Most of my experience is from managed code as well (.net w/ Visual Studio) one thing that I really miss is the ability to debug UnitTests, cases like this are perfect for a TDD approach, we create a test case reproducing the behavior and on VS we just set a few break-points & start debugging...

But so far I have not seen any tool that can help break the VS addiction.

@chris922
Copy link

chris922 commented Jun 1, 2018

Just had the same issue after updating from an older version (I think it was something like 3.0.x) to the newest 3.16.0. Because I had this issue in the Swagger editor as well I found #3376 and thought that the newest Swagger UI shouldn't have this issue.. unfortunately after the upgrade I noticed that this issue is back 😢

Really looking forward for a fix as we are not able to rename the property from properties to something else and thus I have to downgrade again to the old version we had before.

@avb-qlik
Copy link

Same issue I'm facing with Swagger editor live demo here https://editor.swagger.io

@thePanz
Copy link

thePanz commented Aug 3, 2018

Any updates here?

@dalbrx
Copy link

dalbrx commented Aug 12, 2018

The following change in https://github.com/swagger-api/swagger-js seems to fix the issue. However I am not sure if that doesn't arise new problems.

diff --git a/src/specmap/helpers.js b/src/specmap/helpers.js
index 6b3c78a..8ab4480 100644
--- a/src/specmap/helpers.js
+++ b/src/specmap/helpers.js
@@ -36,10 +36,11 @@ const freelyNamedAncestors = [

 export function isFreelyNamed(parentPath) {
   const parentKey = parentPath[parentPath.length - 1]
+  const parentParentKey = parentPath[parentPath.length - 2]
   const parentStr = parentPath.join('/')

-  return (
-    (freelyNamedKeyParents.indexOf(parentKey) > -1) ||
+    return (
+    ((freelyNamedKeyParents.indexOf(parentKey) > -1) && !(freelyNamedKeyParents.indexOf(parentParentKey) > -1)) ||
     (freelyNamedPaths.indexOf(parentStr) > -1) ||
     (freelyNamedAncestors.some(el => parentStr.indexOf(el) > -1))
   )

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants