Skip to content
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

Fix/case sensitive mutability #206

Merged
merged 2 commits into from
Aug 22, 2023
Merged

Conversation

brennanjl
Copy link
Collaborator

Fixed a basic bug where incoming schemas can fail due to having the wrong mutability case

@sonarcloud
Copy link

sonarcloud bot commented Aug 22, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@charithabandi charithabandi merged commit a3c253e into main Aug 22, 2023
2 of 3 checks passed
@charithabandi charithabandi deleted the fix/case-sensitive-mutability branch August 22, 2023 20:08
Copy link
Member

@jchappelow jchappelow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had noticed a MutabilityType enumeration:

const (
MutabilityUpdate MutabilityType = "update"
MutabilityView MutabilityType = "view"
)

Do those values or that type not apply here?

@brennanjl
Copy link
Collaborator Author

Yep, the client sends across a string though (gRPC doesn't restrict values that are not valid enums if they are of the correct underlying type), so we have to manually do the conversion. The bug was where this manual conversion occurs.

@jchappelow
Copy link
Member

I see, gRPC would unmarshal even invalid values.

I guess my question is, what had you planned for MutabilityType? It's enumerated values are unused except in a recently added test, which is why I assumed you intended to use it somewhere around here or in one of the conversion helpers.

Actions: []*transactions.Action{
{
Name: "get_user",
Inputs: []string{"user_id"},
Mutability: transactions.MutabilityUpdate.String(),

I got here because I went looking for const declarations of the "UPDATE" and "VIEW" strings used in the switch (good practice instead of string literals) and I all I found was the enum.

@brennanjl
Copy link
Collaborator Author

Hmm, I think that must've been something that was lost in translation. Looking at it more, it does not seem like we need it.

@brennanjl
Copy link
Collaborator Author

In the engine package, I use enums a lot. pkg/engine/types contains all sorts of enums. The concept of "mutability" doesn't quite translate 1-1 there; the engine treats everything as mutable, and treats immutability as a special case right now. There isn't a great reason for this, other than that it was the easiest way to implement that feature in the engine.

@jchappelow jchappelow added this to the v0.6.0 milestone Sep 28, 2023
brennanjl added a commit that referenced this pull request Feb 26, 2024
* fixed bug where mutability was checked case sensitively

* unincluded changes from other branch
brennanjl added a commit that referenced this pull request Feb 26, 2024
* fixed bug where mutability was checked case sensitively

* unincluded changes from other branch
jchappelow pushed a commit that referenced this pull request Feb 26, 2024
* fixed bug where mutability was checked case sensitively

* unincluded changes from other branch
brennanjl added a commit that referenced this pull request Feb 26, 2024
* fixed bug where mutability was checked case sensitively

* unincluded changes from other branch
brennanjl added a commit that referenced this pull request Feb 26, 2024
* fixed bug where mutability was checked case sensitively

* unincluded changes from other branch
brennanjl added a commit that referenced this pull request Feb 26, 2024
* fixed bug where mutability was checked case sensitively

* unincluded changes from other branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants