Skip to content

Fixes Swagger-UI for Accrual#1237

Closed
thesmallstar wants to merge 1 commit intoapache:developfrom
thesmallstar:fixSwagger1
Closed

Fixes Swagger-UI for Accrual#1237
thesmallstar wants to merge 1 commit intoapache:developfrom
thesmallstar:fixSwagger1

Conversation

@thesmallstar
Copy link
Member

I am trying to try each API in swagger as a part of testing before launching it.
The Accural APIs did not work in tryout, I found a parameter was marked as hidden, I think it is wrong(or maybe I am wrong?).

Link: https://demo.fineract.dev/fineract-provider/swagger-ui/index.html#/Periodic%20Accrual%20Accounting/executePeriodicAccrualAccounting

It works after removing hidden

Copy link
Member

@vorburger vorburger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. What does"hidden" mean here? Do you have a link to the JavaDoc of that annotation? -- 2. Do you know if that JSON Body parameter may contain anything "sensitive", like passwords? -- 3. I wonder if there's a way to automatically test what you seem to be do manually doing (great and thanks!!) - as a future idea, not part of this PR. Just a wild thought.

@ptuomola
Copy link
Contributor

ptuomola commented Aug 6, 2020

Hmm... I thought the idea here was that the actual argument (jsonRequestBody) is hidden, and instead a "pseudo-argument" is created with the correct schema type using the @parameter annotation above the function signature:

@parameter(required = true, schema = @Schema(implementation = AccrualAccountingApiResourceSwagger.PostRunaccrualsRequest.class, description = "Request Body\n"
+ "\n" + "Field Descriptions: \n" + "tillDate: \n" + "which specifies periodic accruals should happen till the given Date"))

So I suppose how this should work is that Swagger would then take the schema from this annotation but send the serialised object as the request body (instead of requiring you to type the jsonRequestBody specifically). But as per your note I'm assuming it doesn't work like that?

Do we actually need both of these parameter definitions? What if we just take the above annotation's arguments and put those to define the jsonRequestBody parameter instead, and above this annotation altogether?

@thesmallstar
Copy link
Member Author

From: https://demo.fineract.dev/fineract-provider/api-docs/apiLive.htm#periodicaccrualaccounting and comment from @ptuomola I believe it is not anything sensitive. Hidden just hides it from the documentation as I concluded from: https://stackoverflow.com/questions/22812365/how-to-hide-a-parameter-in-swagger

@vorburger I have updated to what @ptuomola suggested and this works as expected (tryit button works)

@ptuomola
Copy link
Contributor

ptuomola commented Aug 7, 2020

@thesmallstar great to hear it works! Wonder why it wasn't done like that to start off with... hope there isn't some sort of non-obvious side effect.

Are there lots of others that we need to similarly fix - i.e. is this the pattern that needs to be fixed for every single API to get Swagger to work? And just wondering: why is one of the fields described in the description of the parameter? Shouldn't the field descriptions be in the schema (....Swagger) class?

@thesmallstar
Copy link
Member Author

@ptuomola Only this once had the problem, I am gathering more knowledge on why this is special.

@ptuomola
Copy link
Contributor

@thesmallstar would be great to understand why this is different before we merge, and then fix all in one go. Straightaway I can't see why this would be different...

@ptuomola
Copy link
Contributor

@thesmallstar Finally had some time to look at this! So the problem seems to be that in this one, the request body was annotated with @parameter whereas in all the others it is annotated with @RequestBody. I found another one that is similarly broken: JournalEntriesApiResource.createGLJournalEntry().

So in my view we should make this consistent with the others - i.e. use RequestBody. I will raise a PR to fix - if you can review to see if it makes sense to you, and then I can merge it. Was there a JIRA ticket for this already?

Thanks for looking into this!

@ptuomola
Copy link
Contributor

@thesmallstar PR #1247 - can you have a look?

@thesmallstar
Copy link
Member Author

Closing this one. Thanks for the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants