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

feat(rest): upgrade to AFJ 0.2.0 #120

Closed
wants to merge 13 commits into from

Conversation

amanji
Copy link
Contributor

@amanji amanji commented Jun 15, 2022

TODO: Update PR description

@janrtvld
Copy link
Contributor

Great work!

I think we can remove most of the schemas and use interfaces from AFJ. I think it would be nice if we switched to tsoa as that can generate an OpenAPI spec from the interfaces.

I can do the tsoa implementation if you want. If you need any other help just let me know :).

@amanji
Copy link
Contributor Author

amanji commented Jun 17, 2022

I read the docs briefly and it looks like it would be worth a try. Do you think it's easier to create a branch off of the 0.2.0 branch we can both work off of until everything is ready to be merged?

@janrtvld
Copy link
Contributor

I think the easiest way is to convert the current controllers and tests to work with 0.2.0 and then look at switching to tsoa. We can do this on a separate branch and then merge it when everything works with tsoa.

@TimoGlastra
Copy link
Contributor

I've done some tinkering last week with tsoa and it seems like it doesn't work well with the types as defined in AFJ. I've made some updates in openwallet-foundation/credo-ts#880.

TSOA doesn't always infer types for nested generics/mapped types. I've opened the following issues:

Not sure if there's going to be more problems, but if we decide to adopt TSOA, there's quite a few workarounds we have to apply (mainly redefining interfaces with static types instead of generics).

@janrtvld
Copy link
Contributor

We have to define schemas for every method now (and the openAPI doesnt validate anything nested #17). TSOA works with interfaces so despite the workarounds, wouldn't it still be the better option?

@amanji
Copy link
Contributor Author

amanji commented Jun 22, 2022

An update. I have tests passing for all endpoints except for the basicMessage controller test suite and one test in the connection controller for accept-response. I have opted to skip these until I can determine why they are failing. For connection, I seem to have the flow wrong because the connection record that I am using to accept a connection response is in the wrong state when passing it to the corresponding AFJ method.

@amanji
Copy link
Contributor Author

amanji commented Jun 22, 2022

Before we proceed with adding more endpoints, I think it would be good to resolve whether to move forward with tsoa since this will require updates to the controllers.

@janrtvld
Copy link
Contributor

I discussed it with @TimoGlastra and switching to TSOA seems the best option. Despite its shortcomings, it does provide a better openApi spec than we have now and we can use workarounds for now until they respond to the issues Timo created.

@amanji amanji changed the title REST API Upgrade to AFJ 0.2.0 feat(rest): upgrade to AFJ 0.2.0 Jun 22, 2022
@amanji
Copy link
Contributor Author

amanji commented Jun 22, 2022

Please let me know where I can assist with the TSOA integration. I can help with converting the existing controllers over if we want to start there.

@janrtvld
Copy link
Contributor

I have some time tomorrow to take a whack at it. I'll try and setup tsoa and convert the existing controllers. I'll also take a look at the failing tests.

Maybe you could start on the new controllers?

@amanji amanji deleted the branch openwallet-foundation:afj-0.2.0 June 23, 2022 20:40
@amanji amanji closed this Jun 23, 2022
@amanji amanji deleted the afj-0.2.0 branch June 23, 2022 20:40
@amanji
Copy link
Contributor Author

amanji commented Jun 23, 2022

I renamed the branch, which has inadvertently closed this PR.

@janrtvld I have provided you write access to the petridishdev fork so feel free to make changes there and I can open up a new PR.I can start on the new controllers in the meantime.

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