Skip to content

introduce caveat support in MySQL - #936

Merged
vroldanbet merged 2 commits into
mainfrom
mysql-caveat
Oct 27, 2022
Merged

introduce caveat support in MySQL#936
vroldanbet merged 2 commits into
mainfrom
mysql-caveat

Conversation

@vroldanbet

@vroldanbet vroldanbet commented Oct 25, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@ecordell ecordell left a comment

Copy link
Copy Markdown
Contributor

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!

Comment thread internal/datastore/mysql/migrations/zz_migration.0004_add_caveat.go Outdated
Comment thread internal/datastore/mysql/readwrite.go Outdated
@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

@ecordell ecordell left a comment

Copy link
Copy Markdown
Contributor

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