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
Add mdm_apple_declarative_requests table to log DDM requests #17844
Add mdm_apple_declarative_requests table to log DDM requests #17844
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome! left a few minor comments
server/datastore/mysql/migrations/tables/20240322145615_CreateTableNanoDDMRequests_test.go
Outdated
Show resolved
Hide resolved
server/datastore/mysql/migrations/tables/20240322145615_CreateTableNanoDDMRequests.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, left some comments as things to keep in mind, might apply or not depending on the requirements here. Also, we typically add unit tests for all Datastore methods, though maybe you plan to add them later during the freeze?
server/datastore/mysql/migrations/tables/20240322145615_CreateTableNanoDDMRequests.go
Outdated
Show resolved
Hide resolved
id BIGINT NOT NULL AUTO_INCREMENT, | ||
created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, | ||
enrollment_id VARCHAR(255) NOT NULL, | ||
-- Should be one of "tokens", "declaration-items", "status", or "declaration/…/…" where the ellipses reference a declaration on the server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
server/datastore/mysql/migrations/tables/20240322145615_CreateTableNanoDDMRequests.go
Outdated
Show resolved
Hide resolved
-- Should be one of "tokens", "declaration-items", "status", or "declaration/…/…" where the ellipses reference a declaration on the server | ||
message_type VARCHAR(255), | ||
-- json payload | ||
raw_json TEXT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here for not null. Also heads-up as it's definitely not obvious, TEXT
has smaller limits than MEDIUMTEXT
, to keep in mind depending on the size we expect to have to store here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to specify this as JSON
rather than TEXT
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point, I think Sarah means the JSON
data type that's natively supported by mysql (see e.g. the app_config_json
table).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does MySQL 5.7 support JSON column types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh it looks like yes! I'd be happy to do that if there are no reasons to stick with TEXT
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah just ran into an issue. All but one message_type
have an empty json body, so that validator will complain unless I insert an empty json object
server/datastore/mysql/migrations/tables/20240322145615_CreateTableNanoDDMRequests_test.go
Outdated
Show resolved
Hide resolved
I'm still waiting for the method that accepts a JSON body to be filled out |
…pple_declarative_requests
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat-macos-ddm #17844 +/- ##
=================================================
Coverage ? 65.79%
=================================================
Files ? 1195
Lines ? 109507
Branches ? 2568
=================================================
Hits ? 72052
Misses ? 32001
Partials ? 5454
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
#17792
SELECT *
is avoided, SQL injection is prevented (using placeholders for values in statements)