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

psl: don't crash in ModelWalker after name or type validation errors #4087

Merged
merged 2 commits into from
Jul 28, 2023

Conversation

aqrln
Copy link
Member

@aqrln aqrln commented Jul 27, 2023

Problem description

Fixes: prisma/language-tools#1466

The cause of the problem is that the LSP code action for adding the missing @map attribute for MongoDB relies on the parser database being populated to be able to search for the primary key of the model. However, the parser database constructor returns early when there are validation errors during name and types resolution, so trying to access model attributes later on leads to runtime errors as those structures are not constructed.

There are multiple possible options we could do here:

1. Skip the code action for the @map attribute when there are any other validation errors

We could iterate over the validation errors in the whole document and skip code_actions::mongodb::add_at_map if there is no relevant validation errors before trying to access the model attributes and check the validation errors for a specific span corresponding to the primary field. This might be okay as a hotfix, but would not fundamentally solve the problem and we could run into this again in new code actions we implement.

2. Resolving attributes and relations regardless of validation errors

One option might have been removing early returns and fully populating the parser database, however doing that naively would not work as those early returns are not just an optimization: further validation passes rely on types being valid, and may crash otherwise:

failures:

---- attributes::composite_index::pointing_to_a_non_existing_type stdout ----
thread 'attributes::composite_index::pointing_to_a_non_existing_type' panicked at 'called `Option::unwrap()` on a `None` value', psl/parser-database/src/attributes.rs:632:26
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- attributes::relations::relations_negative::relation_field_in_composite_type_errors stdout ----
thread 'attributes::relations::relations_negative::relation_field_in_composite_type_errors' panicked at 'no entry found for key', psl/parser-database/src/attributes.rs:44:49


failures:
    attributes::composite_index::pointing_to_a_non_existing_type
    attributes::relations::relations_negative::relation_field_in_composite_type_errors

Doing that in a safe way might be possible but would require non-trivial changes in the validation code, and would require further validation passes to not assume the previous ones were successful. I don't have enough context about the validation logic to assess if it's viable.

An upside of this approach would be having more complete validation errors, as multiple problems of different types would be highlighted at the same time. Currently some validation errors will only be reported after fixing other ones.

3. Populating the parser database with stub default values

Instead of trying to eliminate the early returns, we could populate the model attributes with stub default values before returning. As no further validation will happen, and the engines do not work with invalid schemas, this should not lead to incorrect behaviour. However, the code in the language tools should take into account that accessing the model attributes in invalid schemas, although won't lead to crashes anymore, will return incomplete information.

4. Handling missing model attributes in ModelWalker

It's possible to check if model attributes for a model exist in the model_attributes hash map before accessing them, and if not, either construct the default value (this would require returning a Cow instead of a reference, which would impact, e.g., unique_criterias method by forcing it to allocate memory where it previously didn't have to, making the code less efficient) or return a None (changing the contract to return Option there and maybe in some downstream methods to offset the burden of handing this case to the caller would be efficient but would make the API way less ergonomic).

Description of the changes

This PR implements the option 3 by adding the default stubs for model and enum attributes in case of name and type resolution errors, as well as regression tests on the prisma-fmt side.

Fixes: prisma/language-tools#1466

The code action for adding the missing `@map` attribute for MongoDB
relies on the parser database being populated to be able to search for
the primary key of the model. However, the parser database constructor
returns early when there are validation errors during name and types
resolution, so trying to access model attributes later on leads to
runtime errors as those structures are not constructed.

This commit implements populating the model and enum attributes with
stub default values before returning. As no further validation will
happen, and the engines do not work with invalid schemas, this should
not lead to incorrect behaviour. However, the code in the language tools
should take into account that accessing the model or enum attributes in
invalid schemas, although won't lead to crashes anymore, will return
incomplete information.

Fixes: prisma/language-tools#1466
@aqrln aqrln requested a review from a team as a code owner July 27, 2023 18:09
@aqrln aqrln added this to the 5.1.0 milestone Jul 27, 2023
@codspeed-hq
Copy link

codspeed-hq bot commented Jul 27, 2023

CodSpeed Performance Report

Merging #4087 will not alter performance

Comparing fix-prisma-fmt-crash (0daf024) with main (4b132e0)

Summary

✅ 11 untouched benchmarks

@aqrln aqrln changed the title Don't crash in ModelWalker if name or type resolution fails psl: don't crash in ModelWalker if name or type resolution fails Jul 27, 2023
@aqrln aqrln changed the title psl: don't crash in ModelWalker if name or type resolution fails psl: don't crash in ModelWalker after name or type validation errors Jul 27, 2023
Copy link
Contributor

@miguelff miguelff left a comment

Choose a reason for hiding this comment

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

Option 3 looks the most convenient among the ones you thought of to me as well, and the implementation looks good. 👍🏾

@aqrln
Copy link
Member Author

aqrln commented Jul 28, 2023

@miguelff I renamed populate_empty_attributes to create_default_attributes as I think that's a bit better name (it's not exactly clear on the second thought what "empty" meant here).

@aqrln aqrln merged commit b3db4bf into main Jul 28, 2023
51 checks passed
@aqrln aqrln deleted the fix-prisma-fmt-crash branch July 28, 2023 12:53
aqrln added a commit that referenced this pull request Jul 28, 2023
prisma/language-tools#1473 was also fixed by
#4087, this PR adds a
regression test for that issue as well.

Closes: prisma/language-tools#1473
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.

Prisma VS Code extension with mongodb provider crashes when a relation field/type is not defined
2 participants