-
Notifications
You must be signed in to change notification settings - Fork 11
[DMS-877] Design Profiles Feature #770
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
Conversation
4ca521b to
20b9e2b
Compare
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.
Pull request overview
This PR adds comprehensive design documentation and example files for the API Profiles feature in the Ed-Fi Data Management Service. The Profiles feature enables fine-grained control over which data fields are visible or modifiable for different API consumers, supporting use cases like vendor integration, reporting systems, and data privacy compliance. The documentation was generated with assistance from Copilot to provide context for issue #763.
Key Changes:
- Comprehensive design documentation covering architecture, database schema, API specifications, and implementation strategy
- Quick start guide with step-by-step tutorials for using API Profiles
- Five example profile XML files demonstrating common use cases (student, school, assessment, descriptor access patterns)
- XML schema definition (XSD) for validating profile documents
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/Profiles/API-PROFILES-DESIGN.md | Complete 2018-line design document detailing architecture, database schema, API specifications, enforcement pipeline, and implementation tickets for the Profiles feature |
| docs/Profiles/API-PROFILES-QUICKSTART.md | 490-line quick start guide with tutorials, common patterns, testing procedures, and troubleshooting for API Profiles |
| docs/Profiles/examples/profiles/README.md | Overview documentation for example profiles including usage instructions, customization guidelines, and validation procedures |
| docs/Profiles/examples/profiles/student-write-limited.xml | Example profile allowing limited write access to student demographic and contact information |
| docs/Profiles/examples/profiles/student-read-only.xml | Example profile providing read-only access to basic student demographics and school associations |
| docs/Profiles/examples/profiles/school-minimal.xml | Example profile exposing public-facing school directory information |
| docs/Profiles/examples/profiles/descriptor-full-access.xml | Example profile granting complete access to descriptor resources for system administration |
| docs/Profiles/examples/profiles/assessment-limited.xml | Example profile restricting assessment data access to core fields only |
| docs/Profiles/examples/profiles/profile-schema.xsd | XML Schema Definition for validating profile document structure and constraints |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| 1. **Profile Resolution**: Extract profile from `Content-Type` header | ||
| 2. **Rule Evaluation**: Load property/collection/reference rules | ||
| 3. **Input Validation**: |
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.
I don't remember how ODS behaves, but I am wondering about the requirements on the write side. What is the expectation when updating a document using a profile with limited fields? Is the expectation to do a Read-Merge-Write?
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.
Yes, this would be similar to the read/merge/write pattern you mentioned. In ODS, if a field is explicitly marked to be excluded from writing and that field is still sent in the payload, it is simply ignored during the update. The system does not overwrite it with a blank value; instead, it updates only the remaining fields.
|
|
||
| ### Read Operation Enforcement | ||
|
|
||
| For GET operations: |
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.
May be too much of an implementation detail but are we thinking middleware for read projection? Or directly in the query with JSONB operators to project only the profile fields?
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.
The initial idea with the read operation is to apply the filter at the middleware level. This way, the query executes normally, and before assigning the response, the applicable fields are excluded according to the profile definition. This approach provides a more database‑agnostic design regarding JSONB, avoiding dependency on how each engine defines it.
docs/Profiles/API-PROFILES-DESIGN.md
Outdated
|
|
||
| ### Write Operation Enforcement | ||
|
|
||
| For POST/PUT/PATCH operations: |
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.
DMS doesn't support PATCH operations right now. Does ODS?
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.
I understand. Since the use of PATCH is also uncommon in the ODS API, it was only mentioned as a reference but is not included in the endpoint definitions. I’ll remove it for clarity.
simpat-adam
left a comment
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.
My general comment is that the design probably over-explains the config service side and basic CRUD mechanics of the profiles themselves while glossing over the DMS implementation side, (I see one ticket for an "Evaluation Engine"). I posted some comment/questions about the "how" that we might want to address before we dig into tickets.
Add design Files Update files generated by copilot PR to include more context about profiles Add examples
Remove extra sections
Clean the draft
Review and update the notes
Update URL and paths
Update design document
Update request example
Remove unused section Update links
Include table Application Profile Update tickets
c032a1b to
69f9a7d
Compare
Update links
Update diagrams
Fix diagram
Update response
Update descriptions
Regarding the tickets, some may not need to be included, while others might require refinement or splitting. At this stage, we can review suggestions and improvements before creating items in JIRA, ensuring everything is as clear as possible. As for your comments on the tickets related to DMS, the following are listed in the document as part of this section: 3 (Profile Resolution), 4 (Rule Engine), 5 (Write Enforcement), 6 (Read Filtering), 8 (Profile Selection), and eventually 12, which are directly tied to the DMS process. On the Config side, tickets 1, 2, and 7 are included. |
bradbanister
left a comment
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.
Spot-checking this I see a lot of inaccuracies that look like AI slop. How well was this reviewed before submitting as a PR?
|
|
||
| **Application-Based Profile Assignment:** | ||
|
|
||
| When no explicit profile header is provided, DMS: |
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.
This section seems wrong. Profile application depends on relevance to the request resource and usage, ODS returns 403 not 400 and only when there are multiple relevant assigned profiles and no profile header, and I don't know what "falls back to default profile" means. Are default profiles even a thing?
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.
Still needs to address that these are relevant profiles for this request
docs/Profiles/API-PROFILES-DESIGN.md
Outdated
|
|
||
| CallConfigAPI --> ProfileExists{Profile Exists?} | ||
| ProfileExists -->|Yes| LoadProfile[Parse Profile JSON & Rules] | ||
| ProfileExists -->|No| Error404[404 Profile Not Found] |
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.
This is wrong. 406 for GET, 415 for POST/PUT. I don't think Profiles ever returns 404.
docs/Profiles/API-PROFILES-DESIGN.md
Outdated
| <xs:attribute name="memberSelection" type="xs:string"/> | ||
| </xs:complexType> | ||
| </xs:element> | ||
| <xs:element name="WriteContentType" minOccurs="0"> |
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.
Is this intended to be complete? Both this and ReadContentType are missing a lot here.
Remove references to default profile Update Content Type values Update xml schema Update response codes
Update http codes Update examples Update Technical Implementation Details section to be a reference
Update diagrams
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.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Update xml files
| ```bash | ||
| curl -X GET \ | ||
| https://dms-api/data/v3/ed-fi/students/{id} \ | ||
| -H "Accept: application/json;profile=student-read-only" \ |
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.
Wrong
| -H "Authorization: Bearer YOUR_TOKEN" | ||
|
|
||
| # Verify header format | ||
| Accept: application/json;profile=profile-name |
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.
Wrong
| # Test the profile | ||
| curl -X GET \ | ||
| https://dms-api/data/v3/ed-fi/students/{id} \ | ||
| -H "Accept: application/json;profile=student-read-only" \ |
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.
Wrong
| [Auth] -> [ProfileResolution] -> [ParsePath] -> [BuildResourceInfo] -> [ParseBody] -> ... | ||
|
|
||
| Profile Resolution Algorithm: | ||
| 1. Check Accept/Content-Type header for profile parameter |
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.
Wrong
| 1. **Profile Resolution**: Extract profile from `Content-Type` header (Ed-Fi vendor media type format: `application/vnd.ed-fi.{resource}.{profile}.writable+json`) | ||
| 2. **Rule Evaluation**: Load property/collection/reference/filter rules | ||
| 3. **Input Validation**: | ||
| - Check for excluded properties in request body |
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.
Excluded fields are ignored in ODS, what's this for?
| <xs:element name="Object" type="ClassDefinition"/> | ||
| <xs:element name="Collection" type="CollectionDefinition"/> | ||
| </xs:choice> | ||
| <xs:element name="Filter" type="FilterDefinition" minOccurs="0" maxOccurs="unbounded"/> |
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.
ODS has 0 or 1
|
|
||
| ### Profile Not Found | ||
|
|
||
| **Error**: `404 Profile 'xyz' not found` |
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.
406/415
| "collections": [ | ||
| { | ||
| "name": "addresses", | ||
| "memberSelection": "Exclude" |
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.
Is Exclude valid?
| 2. Rename the profile in the `name` attribute | ||
| 3. Modify the `Resource` name if targeting a different resource | ||
| 4. Adjust `memberSelection` strategy (IncludeOnly, ExcludeOnly, etc.) | ||
| 5. Add or remove `Property`, `Collection`, and `Reference` elements |
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.
No Reference in schema
|
|
||
| <!-- Filter: Only include Physical and Mailing addresses --> | ||
| <Filter propertyName="AddressTypeDescriptor" filterMode="IncludeOnly"> | ||
| <Value>Physical</Value> |
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.
Should look like real descriptor values right? Also below
Add design Files.
Design files based on docs generated by copilot PR to include more context about profiles (issue #763
Add examples