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

fix: Differentiate between an object that has a literal * field, and a pointer to an object's additionalProperties shape. #1200

Merged
merged 5 commits into from
Oct 30, 2023

Conversation

jshearer
Copy link
Contributor

@jshearer jshearer commented Sep 26, 2023

This flake was caught by quickcheck (only sometimes) and was causing Flow CI to intermittently fail.

The one thing I'm not confident on is calling this variant AdditionalProperties, since the variant for array's additionalItems is called Token::NextIndex, not Token::AdditionalItems. Thoughts @jgraettinger?


This change is Reviewable

Copy link
Member

@jgraettinger jgraettinger left a comment

Choose a reason for hiding this comment

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

Looks good, some detail comments on specific case handling, and i think we should use NextProperty for the variant.

crates/doc/src/ptr.rs Outdated Show resolved Hide resolved
crates/doc/src/ptr.rs Outdated Show resolved Hide resolved
crates/doc/src/ptr.rs Outdated Show resolved Hide resolved
crates/doc/src/ptr.rs Outdated Show resolved Hide resolved
crates/doc/src/ptr.rs Outdated Show resolved Hide resolved
crates/json/src/lib.rs Outdated Show resolved Hide resolved
@jshearer
Copy link
Contributor Author

Ok @jgraettinger I think this is ready to be looked at again. Changing Token::from_str and adjacent logic to no longer parse - broke a couple tests which I updated to match the new logic, but anything else that depended on that functionality will be broken.. I think the tests should have caught most of those? I also did run a quick e2e test of starting up a local stack, publishing ops catalogs, running a hello-world capture, etc. which all appeared to work

Copy link
Member

@jgraettinger jgraettinger left a comment

Choose a reason for hiding this comment

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

LGTM % some minor nits and adding a bit of removed test coverage back

crates/doc/src/ptr.rs Outdated Show resolved Hide resolved
crates/doc/src/ptr.rs Outdated Show resolved Hide resolved
@@ -502,7 +506,7 @@ mod test {
}

Copy link
Member

Choose a reason for hiding this comment

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

add back the /foo/- case here by manually building up the tokens? You can't implicitly build them by parsing a pointer as it used to do, but you can still build it manually.

Also add a case covering creation with NextProperty. It'd return None, right?

Copy link
Contributor Author

@jshearer jshearer Oct 27, 2023

Choose a reason for hiding this comment

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

Got totally side-tracked and forgot to finished this. What do you mean by adding /foo/- back? That pointer wouldn't make any sense even if I did create it by hand: /foo is an array, and - is interpreted as an object field called -, which you can't use to index into an array array.

Also add a case covering creation with NextProperty. It'd return None, right?

Done

Copy link
Member

Choose a reason for hiding this comment

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

I meant build it up from tokens, rather than parsing, so that it's the NextIndex variant.

It's possible to build such a pointer and pass it to create, right? Let's cover that in the test.

crates/doc/src/shape/limits.rs Outdated Show resolved Hide resolved
…d a pointer to an object's `additionalProperties` shape.

This flake was caught by quickcheck (only sometimes) and was causing Flow CI to intermittently fail.
@jshearer jshearer force-pushed the fix/schema_inference_objects_with_asterisk branch from 23a6c3c to 9864f9a Compare October 27, 2023 20:00
@jshearer jshearer force-pushed the fix/schema_inference_objects_with_asterisk branch from 9864f9a to 1ab6165 Compare October 27, 2023 20:01
@jgraettinger
Copy link
Member

still lgtm!

@jshearer jshearer merged commit b025415 into master Oct 30, 2023
3 checks passed
@jgraettinger jgraettinger deleted the fix/schema_inference_objects_with_asterisk branch October 31, 2023 15:11
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