[Repo Rearchitecture] Future improvement, ironing out an edge case (or two)

[moonlight-komorebi]

Summary

We noticed a potential edge case, pointed out here for validating new / existing quickstarts.

Currently, if a quickstart doesn't have an id, we assume its a new quickstart and validate it using a mock id. There is a possibility that someone removes an id from an existing quickstart, commits that, and down the line we then create a new quickstart (because we would generate a new id), even though that quickstart already exists.

I think we want to be able to detect:

1. if a quickstart is new, and therefore we can validate with a mock id.
2. is there a change to the id field of an existing quickstart (removing the id), in which case we would NOT want to validate using mock id. in this case we want to send null / empty id and have the api tell us we are missing an id.

Furthermore, it might even be worth implementing a robust check to determine if someone submits a new quickstart with an id, or if someone modifies an existing quickstart's id with another quickstart's id.

Acceptance Criteria

• [ ] High level: prevent this edge case from occurring by improving validation script / process.

[jerelmiller]

we would prefer the api tell us we are missing an id.

Can you define "missing" here? Does that mean the value is null? An empty string? The mock UUID (with all 0s)?

Assuming the answer is that missing == null, then this is sorta technically there now, albeit it happens as a part of the GraphQL type checking rather than app logic. This is a non-nullable field in the GraphQL API so you'll get a GraphQL type error when trying to submit the mutation before it hits our service.

It's important to note as well that we validate that the quickstart's name is unique. While not fully guaranteed to catch all use cases, it should theoretically catch the case that someone deleted an id on a quickstart that was already created and tried to resubmit; at least assuming the name of the quickstart did not change in the process.

Unfortunately doing much beyond that is very tricky to do API-side. Because the service is source agnostic (we don't couple it to the public repo), it becomes difficult to determine intent behind the request. Did the user mean to try and generate a new one by deleting an ID? Was it a mistake? Is the user actually trying to update an existing one? etc. All the service can do is say "yep we have a quickstart with this ID, go update it", or "nope, this ID doesn't exist, go try and create it". It has a difficult time determining if the request should be deemed a mistake (like deleting an id in the config file, only to regenerate it later). I think this is a problem best solved on this repo as it has the full landscape of all quickstarts and can better grasp the intent of a modification.

That being said, I'm not sure its the end of the world if this happens. Long term we'd like to build more robust quickstart management tools, especially tools that control visibility of a quickstart. Once we are able to realize that future, this problem becomes mostly obsolete since we will have the tools to easily correct mistakes in the event someone deleted an id, changed the name of the quickstart, then it got submitted again.

---

Despite all the above, it might be a good idea to consider adding a validation on the service to ensure we don't accept the "mock" quickstart ID as an actual ID. I could see that being something the service does for us.

[github-actions[bot]]

Old issues will be closed after 105 days of inactivity. This issue has been quiet for 90 days and is being marked as stale. Reply here to keep this issue open.

[github-actions[bot]]

This issue is being closed due to inactivity. Is this a mistake? Please re-open this issue or create a new one.