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

graph-api: added more validation #515

Merged
merged 1 commit into from
Sep 18, 2024
Merged

graph-api: added more validation #515

merged 1 commit into from
Sep 18, 2024

Conversation

aramikm
Copy link
Contributor

@aramikm aramikm commented Sep 13, 2024

Problem

Adds extra validation on graph-api dtos and request/path params

Closes #496

Discussion

  • Since we are going to overhaul our way of e2e testing it didn't make sense to add tests which would be replaced in the future.

Steps to Verify:

  1. Run the api
  2. Go to swagger
  3. Try some invalid values in requests

@@ -15,7 +16,7 @@ export class WatchGraphsDto {

@IsNotEmpty()
@IsString()
@IsUrl({ require_tld: false })
@IsUrl({ require_tld: false, require_protocol: true, require_valid_protocol: true })
Copy link
Contributor

Choose a reason for hiding this comment

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

These are good to know 💡

Copy link
Contributor

@saraswatpuneet saraswatpuneet left a comment

Choose a reason for hiding this comment

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

👍🏽

Copy link
Contributor

@mattheworris mattheworris left a comment

Choose a reason for hiding this comment

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

  • Read through the code
  • Looks good, agree with other comments

🚢 it

@JoeCap08055
Copy link
Contributor

Regarding e2e tests: if we have a working e2e test framework, then it's worth while to add tests, because the test scenarios themselves for endpoint validation won't change--only the test framework setup will change, and we'll add additional tests for worker scenarios.

But if you want to add tests in a separate PR, that's fine.

@aramikm
Copy link
Contributor Author

aramikm commented Sep 18, 2024

I think it would makes sense to add those tests when the framework is ready simply due to improved velocity of writing and verifying them compared to current state that if something goes wrong I would have to dig to see if my written test is the issue or the framework setup.

@aramikm aramikm merged commit 097b7af into main Sep 18, 2024
13 checks passed
@aramikm aramikm deleted the graph_api_validation branch September 18, 2024 21:08
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.

graph-api endpoints data syntax verification
4 participants