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

Restructure API routes for other application types #452

Closed
Tracked by #441 ...
IanWearsHat opened this issue Dec 9, 2024 · 5 comments · Fixed by #474
Closed
Tracked by #441 ...

Restructure API routes for other application types #452

IanWearsHat opened this issue Dec 9, 2024 · 5 comments · Fixed by #474

Comments

@IanWearsHat
Copy link
Member

image
Workaround is to make other routes with specified RawApplicationData types.

@taesungh
Copy link
Member

taesungh commented Dec 9, 2024

If we wanted to have it all under one route: I think what we'll need to do in that case is use a concrete model which contains the union of all application fields so that FastAPI recognizes what values should be parsed from the form data. Then inside the route, use the application type as a discriminator to attempt validation on a specific model (see Discriminated Unions with str discriminators)

@taesungh
Copy link
Member

taesungh commented Dec 9, 2024

Think also duplicate of #448?

@waalbert
Copy link
Contributor

waalbert commented Dec 9, 2024

We've made the decision to add separate routes for mentor and volunteer applications. In the future, we can take a look into combining all the applications into one endpoint.

@taesungh
Copy link
Member

taesungh commented Dec 9, 2024

Actually, I agree: separate routes keeps it clear what data each form needs (e.g. when looking at the auto-generated API documentation for debugging as I often do). Where a discriminator could help is the final step of constructing the ProcessedApplicationData object as well as the Applicant object.

@waalbert
Copy link
Contributor

waalbert commented Dec 9, 2024

A little bit of misunderstanding earlier. I see what you're saying, and I agree.

@taesungh taesungh changed the title FastAPI routes don't allow for Unioned RawApplicationData Restructure API routes for other application types Dec 9, 2024
@taesungh taesungh added this to the Release Applications milestone Dec 9, 2024
@IanWearsHat IanWearsHat linked a pull request Dec 11, 2024 that will close this issue
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 a pull request may close this issue.

3 participants