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.

Sign up for GitHub

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

Jump to bottom

introduce caveat support in MySQL #936

Merged
merged 2 commits into from
Oct 27, 2022
Merged

introduce caveat support in MySQL #936

merged 2 commits into from
Oct 27, 2022

Conversation

vroldanbet
Copy link
Contributor

@vroldanbet vroldanbet commented Oct 25, 2022 •
edited
Loading

Closes #918

What

Implements support for caveats and caveated relationships in MySQL datastore

How

  • The strategy is not very different from other datastores, the implementation is similar to Postgres'
  • MySQL JSON column was used to align with other datastores. Unfortunately MySQL's go driver does not come with built-in marshalling/unmarshalling of maps into the JSON datatype, so a wrapper struct was created to implement the Valuer and Scanner interfaces the driver defines to allow custom data types to be marshalled/unmarshalled.

Some additional changes to Postgres datastore

I introduced some improvements into Postgres datastore which I have missed in #890:

  • context severing was not in place
  • do not construct OR condition by looping - squirrel turns slices into an IN clause
  • cosmetic adjustment to errors, and turning them into constants
  • missing error wrapping in some return statements
  • redundant EQ condition in deleteCaveatsWithClause

@github-actions github-actions bot added the area/datastore Affects the storage system label Oct 25, 2022
@github-actions github-actions bot added area/api v1 Affects the v1 API area/dependencies Affects dependencies area/dispatch Affects dispatching of requests area/schema Affects the Schema Language area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Oct 25, 2022
@github-actions github-actions bot removed area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) area/api v1 Affects the v1 API area/schema Affects the Schema Language area/dependencies Affects dependencies area/dispatch Affects dispatching of requests labels Oct 25, 2022
@vroldanbet vroldanbet force-pushed the mysql-caveat branch 3 times, most recently from 388a66a to 48c9291 Compare October 25, 2022 17:17
@vroldanbet vroldanbet self-assigned this Oct 25, 2022
@vroldanbet vroldanbet marked this pull request as ready for review October 26, 2022 08:24
@vroldanbet vroldanbet requested a review from a team October 26, 2022 08:24
@vroldanbet vroldanbet force-pushed the mysql-caveat branch 7 times, most recently from 4f3ffbc to 80fd257 Compare October 27, 2022 07:26
@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Oct 27, 2022
@vroldanbet vroldanbet enabled auto-merge October 27, 2022 07:55
@vroldanbet vroldanbet force-pushed the mysql-caveat branch 6 times, most recently from c829c1e to 322d09f Compare October 27, 2022 09:50
@github-actions github-actions bot added the area/CLI Affects the command line label Oct 27, 2022
@vroldanbet vroldanbet force-pushed the mysql-caveat branch 2 times, most recently from 8a35cda to 80fd257 Compare October 27, 2022 10:37
@github-actions github-actions bot removed the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Oct 27, 2022
ecordell
ecordell reviewed Oct 27, 2022
View reviewed changes
Copy link
Contributor

@ecordell ecordell left a comment

Choose a reason for hiding this comment

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

just a couple of small things, looks good!

@vroldanbet
some improvements in Postgres caveat implementation
dadbaac 
- context severing was not in place
- do not construct Or by looping (squirrel handles it)
- cosmetic adjustment to errors
- missing error wrapping in some return statements
- redundant EQ condition in deleteCaveatsWithClause
- introduce constants for error wrapping message
@vroldanbet vroldanbet requested a review from ecordell October 27, 2022 15:59
ecordell
ecordell approved these changes Oct 27, 2022
View reviewed changes
Copy link
Contributor

@ecordell ecordell left a comment

Choose a reason for hiding this comment

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

LGTM

@vroldanbet vroldanbet merged commit e837bc3 into main Oct 27, 2022
@vroldanbet vroldanbet deleted the mysql-caveat branch October 27, 2022 16:07
@github-actions github-actions bot locked and limited conversation to collaborators Oct 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ecordell ecordell ecordell approved these changes

Assignees

@vroldanbet vroldanbet

Labels
area/CLI Affects the command line area/datastore Affects the storage system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support caveats in MySQL datastore
2 participants
@vroldanbet @ecordell