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: Early return identifier fields if view matches. #154

Merged
merged 2 commits into from
May 23, 2019

Conversation

AllPurposeName
Copy link
Contributor

@AllPurposeName AllPurposeName commented May 21, 2019

Before this when we passed in view: :identifier, we were incorrectly adding sorted fields to the identifier fields. Sorted fields would include all fields and associations not scoped to a specific view.

The expected behavior is that if view == :identifier, then only the identifier fields will be returned.

Thus,

# Setup
class MyBlueprint < Blueprinter::Base
  identifier :id
  field :first_name

  view :normal do
    field  :last_name
  end
end
# Old behavior
MyBlueprint.render(obj, view: identifier) #=> { "id": 1, "first_name": "Rock" }

vs

# New behavior
MyBlueprint.render(obj, view: identifier) #=> { "id": 1 }

Before this we were incorrectly adding 'sorted fields' to the identifier
fields. Sorted fields would include all fields and associations not
scoped to a specific view.

Now the correct behavior is that if view == :identifier, then only the
identifier fields will be returned.
Copy link
Contributor

@mcclayton mcclayton left a comment

Choose a reason for hiding this comment

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

Looks good to me, this will be merged into: https://github.com/procore/blueprinter/tree/1.0.0-rc
which we will use to accumulate our breaking changes for 1.0.0 version release.

@philipqnguyen
Copy link
Contributor

Would this be a bugfix rather than a breaking change?

It seems to me that if I specify :identifier but I get in return fields outside of :identifier then it is a bug, right?

@mcclayton
Copy link
Contributor

@philipqnguyen It's certainly a bug fix, but we were thinking this may be a breaking bug fix as we cannot guarantee that any users are not referencing identifier view fields exposed as part of the bug.

@philipqnguyen
Copy link
Contributor

Then would every bugfix be breaking? Since we cannot guarantee that people weren't using a bugged feature?

@dlcarter
Copy link
Contributor

I agree with @philipqnguyen that this is a bug fix, and not a breaking change. I can't imagine anyone rightfully running into this bug and relying on it.

@AllPurposeName AllPurposeName changed the base branch from 1.0.0-rc to master May 23, 2019 18:33
@mcclayton
Copy link
Contributor

Discussed more offline, we determined that this is a bugfix, and not a 1.0.0 release candidate. In 1.0.0, we will determine whether or not we even want to keep the Blueprinter defined identifier view around.

@AllPurposeName
Copy link
Contributor Author

Changed the branch, I will resolve conflicts.

@mcclayton mcclayton merged commit e775967 into master May 23, 2019
@mcclayton mcclayton deleted the dj/remove-duplicate-identifier-fields branch May 23, 2019 18:47
@mcclayton mcclayton mentioned this pull request May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants