Skip to content
This repository has been archived by the owner on Oct 18, 2024. It is now read-only.

Cannot return null on non-nullable field #83

Closed
MinhxNguyen7 opened this issue Aug 23, 2023 · 4 comments · Fixed by #85
Closed

Cannot return null on non-nullable field #83

MinhxNguyen7 opened this issue Aug 23, 2023 · 4 comments · Fixed by #85
Assignees
Labels
area: graphql-api Related to the GraphQL API specifically priority: high High priority issue status: confirmed The bug has been reproduced and is confirmed story point: 1 Quick copy change type: bug Something isn't working

Comments

@MinhxNguyen7
Copy link
Member

MinhxNguyen7 commented Aug 23, 2023

I think this should work. The structure worked for other queries but not this one, and it's not clear to me why that's the case. Everything works as long as the instructor's name isn't provided, which is what AntAlmanac has been using until now, hence why this was not discovered.

{ 
        aggregateGrades(department: "I&C SCI", courseNumber: "45C", instructor: "SHINDLER, M.") {
            gradeDistribution {
                gradeACount
                gradeBCount
                gradeCCount
                gradeDCount
                gradeFCount
                gradePCount
                gradeNPCount
                averageGPA
            }
        },
    }

I'm marking this as high because it breaks the feature being introduced to Antalmanac (icssc/AntAlmanac#671)

@MinhxNguyen7 MinhxNguyen7 added priority: high High priority issue area: graphql-api Related to the GraphQL API specifically type: bug Something isn't working labels Aug 23, 2023
@ecxyzzy ecxyzzy self-assigned this Aug 23, 2023
@ecxyzzy ecxyzzy moved this from Backlog to Needs Review in PeterPortal API :: Next Aug 23, 2023
@ecxyzzy
Copy link
Member

ecxyzzy commented Aug 23, 2023

There's nothing wrong with the structure, the issue is that this query returns an empty set of sections, and we aggregate the data by transforming the raw data. I'm pretty sure this is because I forgot to account for this case, and NaN (result of dividing by the section list length, which might be zero) got coerced to null.

Time to add this case to #81 as a regression test 🖖

@ecxyzzy ecxyzzy added story point: 1 Quick copy change status: confirmed The bug has been reproduced and is confirmed labels Aug 23, 2023
@MinhxNguyen7
Copy link
Member Author

MinhxNguyen7 commented Aug 24, 2023

How come it returns an empty set? That would imply that Shindler has never taught ICS 45

@ecxyzzy
Copy link
Member

ecxyzzy commented Aug 24, 2023

That is correct; as of right now, he has never taught ICS 45C. I believe he's scheduled to teach it in the fall, but we won't have the grade distribution info for that until winter.

@MinhxNguyen7
Copy link
Member Author

Oh yeah I'm being dumb. I was thinking of 46

@github-project-automation github-project-automation bot moved this from Needs Review to Done in PeterPortal API :: Next Aug 26, 2023
ecxyzzy added a commit that referenced this issue Aug 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: graphql-api Related to the GraphQL API specifically priority: high High priority issue status: confirmed The bug has been reproduced and is confirmed story point: 1 Quick copy change type: bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants