Define report schema in more detail#29
Conversation
WalkthroughTightens public OpenAPI schemas: replaces permissive object shapes with explicit properties, required fields, and closed objects ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
public/openapi/participant-api.json(2 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
public/openapi/participant-api.json
[error] 3656-3656: Expected a property but instead found '}'.
Expected a property here.
(parse)
🔇 Additional comments (2)
public/openapi/participant-api.json (2)
3619-3637: LGTM! Well-defined pagination schema.The explicit property definitions with
additionalProperties: falseimprove type safety and API contract clarity.
3643-3689: Good schema definition; approve after fixing the syntax issues.The explicit Report schema with detailed properties, required fields, and
additionalProperties: falsesignificantly improves type safety and API contract clarity. The nesteddatastructure is well-defined with appropriate constraints.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
public/openapi/management-api.json (1)
833-839: TightenedReportschema looks good; consider wiringdataitems toReportDatafor consistencyMaking
key,participantID,responseID, andtimestamprequired plus adding the description is a solid improvement and aligns with how reports are used elsewhere in this spec. One optional follow-up you might consider (either here or in a later PR) is to reuse the existingReportDataschema forReport.data.itemsso that the list endpoint and the update endpoint share the same, fully-typed item shape.For example:
Optional: reuse
ReportDatainReport.data"data": { "type": "array", - "items": { - "type": "object", - "additionalProperties": true - } + "items": { + "$ref": "#/components/schemas/ReportData" + } }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
public/openapi/management-api.json (1)
804-830: Clarify timestamp unit forReport.timestampThe
timestampfield now hastype: integerwith description"Unix timestamp", but other places (e.g.,expiresAt) explicitly say"Unix timestamp (seconds)". To avoid ambiguity for clients and tooling, consider specifying whether this is seconds or milliseconds.Example tweak
- "timestamp": { - "type": "integer", - "description": "Unix timestamp" - }, + "timestamp": { + "type": "integer", + "description": "Unix timestamp (seconds)" + },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
public/openapi/management-api.json(3 hunks)
🔇 Additional comments (1)
public/openapi/management-api.json (1)
833-839:Reportrequired fields look consistentMarking
key,participantID,responseID, andtimestampas required matches the defined properties and will help consumers rely on these fields always being present. No issues from a schema perspective.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
public/openapi/management-api.json (1)
804-851: ClarifyReport.requiredfields vs documented optionalresponseIDThe
Report/ReportDataschemas look good (closed object, sharedReportDataref, constraineddtypeenum). However, there's a mismatch between the schema and documentation: in the OpenAPI schema,responseIDis listed as required, but the documentation describes it as "Identifier of the originating response or event, if any" and does not mark it as required in the TypeTable.If
responseIDis genuinely optional (not always present for all reports), remove it from therequiredarray:"required": [ "key", "participantID", "timestamp" ]Alternatively, update the documentation to mark
responseIDas required and clarify that it is always present.
🧹 Nitpick comments (3)
public/openapi/participant-api.json (3)
3622-3636: Consider marking pagination properties as required.All four pagination properties (totalCount, currentPage, totalPages, pageSize) appear essential for pagination functionality. Without a
requiredarray, the schema permits responses with incomplete pagination info.🔎 Proposed refinement
"PaginationInfos": { "type": "object", "description": "Pagination information", "properties": { - "totalCount": { - "type": "integer" - }, - "currentPage": { - "type": "integer" - }, - "totalPages": { - "type": "integer" - }, - "pageSize": { - "type": "integer" - } + "totalCount": { + "type": "integer" + }, + "currentPage": { + "type": "integer" + }, + "totalPages": { + "type": "integer" + }, + "pageSize": { + "type": "integer" + } }, + "required": [ + "totalCount", + "currentPage", + "totalPages", + "pageSize" + ], "additionalProperties": false }
3647-3650: Consider addingidto required fields.The
idfield represents the MongoDB ObjectID and is typically always present. Adding it to therequiredarray ensures API clients can rely on its presence.🔎 Proposed refinement
"required": [ + "id", "key", "participantID", "responseID", "timestamp" ],
3670-3680: Tighten data item schema with required fields and additionalProperties constraint.The data items lack a
requiredarray andadditionalPropertiesconstraint. Given that the PR tightens schemas with explicit properties andadditionalProperties: falsefor bothPaginationInfosandReport, the data items appear inconsistent.🔎 Proposed refinement
"data": { "type": "array", "items": { "type": "object", "properties": { "key": { "type": "string" }, "value": { "type": "string" }, "dtype": { "type": "string", "enum": ["date", "float", "int", "string", "rawMessage", "keyList"] } - } + }, + "required": ["key", "value", "dtype"], + "additionalProperties": false } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
content/docs/participant-management/reports.mdx(1 hunks)public/openapi/management-api.json(3 hunks)public/openapi/participant-api.json(2 hunks)
🔇 Additional comments (2)
content/docs/participant-management/reports.mdx (1)
41-44:modifiedAtdocs align with the API schemaThe new
modifiedAtfield is documented as an optional string date-time of the last modification, which matches the OpenAPIReport.modifiedAt(string,date-time, not required). Looks consistent and clear.public/openapi/participant-api.json (1)
3654-3656: Past issues resolved.The trailing comma syntax error previously flagged at line 3656 and the duplicate "string" value in the
dtypeenum at line 3677 have both been resolved. The enum now correctly uses "date" as the first value.Also applies to: 3675-3678
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
public/openapi/management-api.json (2)
817-820: Optional: alignReport.timestampwording with other Unix timestamp fieldsYou describe this as
"Unix timestamp", while elsewhere (e.g. auth token fields) you use"Unix timestamp (seconds)". If this is also seconds, consider mirroring that phrasing for consistency; if it differs, calling out the unit explicitly would help clients.
825-838: SharedReportDataschema and stricterReportshape look good; just verify enum/requireds and consider a bit more docsMoving
Report.data.itemsto#/components/schemas/ReportDataand settingadditionalProperties: falseis a nice tightening of the contract, and adding"date"plus deduplicating the enum values ondtypebetter reflects actual usage.A couple of follow‑ups to consider:
Report.required = ["key", "participantID", "timestamp"]is a stronger guarantee than before; please double‑check that all existing responses (including legacy data) always include these three fields so the schema does not over‑promise.- Adding
"date"to thedtypeenum is an expansion; generated clients that modeled the old enum as a closed set may need to be regenerated. If this field has been returning"date"in production already, it would be good to call that out in changelog/release notes.- For
ReportData, a short description onkey,value, and especiallydtype(including how non‑string dtypes likeint,float,date,rawMessage,keyListare encoded into thestringvalue) would make the schema much more self‑explanatory to client authors.Also applies to: 840-851
public/openapi/participant-api.json (1)
3619-3637: Consider adding required fields to PaginationInfos.The
PaginationInfosschema properly constrains the structure with explicit properties andadditionalProperties: false, but all fields are optional. Pagination metadata typically requires these fields to be meaningful to clients.🔎 Suggested improvement
"pageSize": { "type": "integer" } }, + "required": [ + "totalCount", + "currentPage", + "totalPages", + "pageSize" + ], "additionalProperties": false
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/openapi/management-api.json(3 hunks)public/openapi/participant-api.json(2 hunks)
🔇 Additional comments (2)
public/openapi/management-api.json (1)
804-807: ClarifiedReport.iddescription is consistent and helpfulUsing
"MongoDB ObjectID as hex string"matches other schemas (e.g.ManagementUser.id) and makes the wire format unambiguous. No further changes needed here.public/openapi/participant-api.json (1)
3643-3688: Excellent schema definition with proper constraints.The
Reportschema is well-structured with:
- Explicit property definitions and helpful descriptions
- Appropriate type constraints (integer for timestamp, date-time format for modifiedAt)
- Required fields for core identifiers (key, participantID, timestamp)
- Strict validation with
additionalProperties: falseThis significantly improves API contract clarity compared to the previous permissive schema.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
public/openapi/participant-api.json (1)
3668-3682: Add constraints onReport.dataarray items.The
dataarray items lack structural constraints: norequiredfields and noadditionalPropertiessetting at the items level. This allows empty objects or objects with arbitrary properties in the array, undermining the strict schema approach used elsewhere in this PR.🔎 Recommended fix to enforce item structure
"data": { "type": "array", "items": { "type": "object", "properties": { "key": { "type": "string" }, "value": { "type": "string" }, "dtype": { "type": "string", "enum": ["date", "float", "int", "string", "rawMessage", "keyList"] } - } + }, + "required": ["key", "value", "dtype"], + "additionalProperties": false } }
🧹 Nitpick comments (1)
public/openapi/management-api.json (1)
804-838: Confirm ifdatashould be required onReportYou’ve now closed
Report(additionalProperties: false) and markedkey,participantID, andtimestampas required, but notdata. If the backend rejects reports without anydata, the schema will be more accurate and consumer-friendly ifdatais also required.If empty
datais allowed, this is fine as-is; otherwise, consider:Proposed change to make
datarequired (if aligned with backend)"description": "Report object", - "required": [ - "key", - "participantID", - "timestamp" - ] + "required": [ + "key", + "participantID", + "timestamp", + "data" + ]
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/openapi/management-api.jsonpublic/openapi/participant-api.json
🔇 Additional comments (1)
public/openapi/participant-api.json (1)
3619-3637: LGTM! Schema properly tightened.The
PaginationInfosschema now explicitly defines all properties with appropriate integer types and closes the object withadditionalProperties: false, preventing arbitrary fields. This aligns with the PR's goal of providing more detailed schemas.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
public/openapi/management-api.json (2)
804-838: Report schema looks good; consider marking response-only fields as read‑only / clarifying redundancyThe
Reportdefinition is now much clearer (iddescription, explicittimestamp,dataasReportData[], and required core fields). Two optional refinements you might consider later:
- Mark fields that are response-only (e.g.
id, possiblymodifiedAt) as"readOnly": trueso client SDKs don’t expect them on create/update payloads.- Since
participantIDis already in the path for/participants/{participantID}/reports, decide whether you really want it required in the body as well, or if it should be documented as redundant/ignored when mismatching.Both are non-breaking documentation polish items and can be deferred.
840-850: TightenReportDatacontract (required fields and value typing) for better tooling
ReportDatais now a closed object with explicitkey,value, anddtypeenum, which is a big improvement. Two follow-ups you might want to add:
- Make the structure explicit by requiring these fields:
Optional
requiredclause"ReportData": { "type": "object", "properties": { "key": { "type": "string" }, "value": { "type": "string" }, "dtype": { "type": "string", "enum": ["date", "float", "int", "string", "rawMessage", "keyList"] } }, - "additionalProperties": false + "additionalProperties": false, + "required": ["key", "value", "dtype"] },
- Optionally clarify in a description whether
valueis always a string representation (even forfloat/int/date) or consider a more preciseoneOftyping similar toStudyVariables.valueif you want strict type guarantees.These are schema-hardening/documentation improvements, not blockers.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/openapi/management-api.jsonpublic/openapi/participant-api.json
🚧 Files skipped from review as they are similar to previous changes (1)
- public/openapi/participant-api.json
2 participants
For the endpoint: /v1/study-service/participant-data/{studyKey}/reports
Define the response schemas in more detail.
Summary by CodeRabbit
API Changes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.