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

Refactor jobs restapi to remove wtforms #339

Merged
merged 1 commit into from
Dec 15, 2023

Conversation

alexb1200
Copy link
Contributor

Addresses issue #313, removes the wtforms dependency, refactors the controller to accept the job form, and changes _client.py and client.py to reflect those changes. Makes minimal changes to the jobs unit tests to reflect those updates.

@jkglasbrenner
Copy link
Collaborator

#328 has been merged. I just squashed and rebased this branch/pull request to bring this in sync with the latest changes in dev.

@jkglasbrenner
Copy link
Collaborator

While doing a code review, I found that the Swagger documentation wasn't coming out right, even after using load_only and dump_only on the Marshmallow Schema. The issue is that form_schema= argument for the @accepts decorator is processed differently, and basically it ignores load_only and dump_only when generating the Swagger Docs.

I'm working on a way to fix this without needing to patch the flask_accepts package. Somewhat related to the above, I'm also fixing things so that you can access the form submitted to POST using request.parsed_form and so that everything can be handled with just JobSchema so that we can delete JobSubmitSchema. When I get these worked out, I'll push the update to this branch.

Finally, I want to verify that the file upload works not just in the unit tests but also when I build and run the full deployment.

@jkglasbrenner jkglasbrenner force-pushed the abyrne-refactor-jobs-restapi branch 3 times, most recently from 6376fa5 to 6d12f9c Compare December 14, 2023 23:24
@jkglasbrenner
Copy link
Collaborator

I've implemented the changes so that we can use form_schema= with a Marshmallow Schema that makes use of dump_only and load_only, and allows us to generate the correct Swagger docs. Here's what the decorators on the post function look like now:

@api.expect(
as_api_parser(
api,
as_parameters_schema_list(JobSchema, operation="load", location="form"),
)
)
@accepts(form_schema=JobSchema, api=api)

The new part is the as_parameters_schema_list function, which takes the Marshmallow schema and converts it to the format needed in the as_api_parser function. The as_parameters_schema_list respects the dump_only and load_only settings, so the documentation for the POST endpoint now comes out correct:

image

Also, just to document a "gotcha" with the difference between how the requests package works and the Flask test client works:

I had to go back and restore the files= argument in the requests.post method used in the Dioptra client, as it's the only way to specify a file for uploading:

job_files = {"workflow": (workflows_file.name, f)}
response = requests.post(
self.job_endpoint,
data=job_form,
files=job_files,
)

It is different from the Flask test client, which doesn't use the files= argument:

with app.test_client() as client:
response: Dict[str, Any] = client.post(
f"/api/{JOB_BASE_ROUTE}/",
content_type="multipart/form-data",
data=job_form_request,
follow_redirects=True,
).get_json()

@jkglasbrenner
Copy link
Collaborator

As long as the GitHub Actions tests all pass, this is ready to merge.

In terms of merge order, I'm going to review and merge the fix in #342 first to dev, since it also touches the Job endpoint. Then I'm going to resolve any conflicts that introduces in this branch, and once that's handled, merge this in.

Copy link
Collaborator

@jkglasbrenner jkglasbrenner left a comment

Choose a reason for hiding this comment

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

Rebased this branch after merging #342 to dev, all unit tests and code quality checks passed. Looks good to me for a merge.

@jkglasbrenner jkglasbrenner merged commit afeb347 into dev Dec 15, 2023
21 checks passed
@jkglasbrenner jkglasbrenner deleted the abyrne-refactor-jobs-restapi branch December 15, 2023 15:32
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.

2 participants