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

Slight Optimization to Logging and Paging Endpoints and Parameterized SQL Queries #681

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

WarpWing
Copy link
Contributor

@WarpWing WarpWing commented Nov 8, 2023

Pull Request Description
Made some slight formatting/optimization fixes to the FarmData2 Logging Endpoint. In addition, I also parameterized(in lieu of concatenation) SQL queries so that user input is treated as data, not as part of the SQL command itself. This prevents SQL injection and stops unauthorized commands from being run via the endpoint. Prepared statements utilize ? as placeholders for logType, startTime, endTime, startIndex, and limit. I also removed pagination and instead handled that in the LIMIT section of the query.

I'm currently putting this in a "DRAFT" pull request because I would like to test out some more behaviors before making this reviewable.

Licensing Certification

FarmData2 is a Free Cultural Work and all accepted contributions are licensed as described in the LICENSE.md file. This requires that the contributor holds the rights to do so. By submitting this pull request I certify that I satisfy the terms of the Developer Certificate of Origin for its contents.

Signed-off-by: Ty Chermsirivatana <[email protected]>
Signed-off-by: Ty Chermsirivatana <[email protected]>
@WarpWing WarpWing changed the title Slight Optimization to Logging Endpoint and Parameterized SQL Queries Slight Optimization to Logging and Paging Endpoints and Parameterized SQL Queries Nov 8, 2023
@WarpWing
Copy link
Contributor Author

WarpWing commented Nov 8, 2023

In addition, just looked at /paging, and made some small refactoring including pagination via implementing LIMIT in the SQL statement.

@braughtg
Copy link
Member

braughtg commented Nov 8, 2023

Thanks for your work on this! These look like nice updates.

I don't see any obvious breaking changes, but as you work be sure to double check both manually and by running the tests in farmdata2_api/cypress. The API is also used by functions in farmdata2_modules/resources/FarmOSAPI.js so be sure to run the tests in FarmOSAPI.spec.js as well.

@WarpWing
Copy link
Contributor Author

WarpWing commented Mar 2, 2024

I forgot this fossil PR lying around but I tested these changes a while back and they all looked good on my end. Just never marked it ready for review given fall midterms.

@WarpWing WarpWing marked this pull request as ready for review March 2, 2024 02:26
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