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

Make sure passed ids for habtm relation are unique #15651

Conversation

Ladas
Copy link
Contributor

@Ladas Ladas commented Jul 25, 2017

Make sure passed ids for habtm relation are unique, otherwise it could lead to duplication in the relation table or unique index constraint violation.

-- it's enhancement in a sense, that we don't need to filter the data in the parser

@Ladas
Copy link
Contributor Author

Ladas commented Jul 25, 2017

@miq-bot assign @agrare
@miq-bot add_label enhancement
cc @pkliczewski

@agrare
Copy link
Member

agrare commented Jul 25, 2017

@Ladas did you push the wrong branch? This looks like the change from #15574 and the spec from IDK what 😆

@agrare
Copy link
Member

agrare commented Jul 25, 2017

Great commit messages though :D

@Ladas
Copy link
Contributor Author

Ladas commented Jul 26, 2017

@agrare damned, yes. But the spec is great :-) Let me push the right first commit. :-)

Ladas added 2 commits July 26, 2017 09:31
Test we take only unique ids for the HABTM relations, if not
unique, the relation table could have duplicates or fail the
unique constraint.
Make sure that passed ids for HABTM relation are uniq
@Ladas Ladas force-pushed the make_sure_passed_ids_for_habtm_relation_are_unique branch from 669c112 to 0a3faf3 Compare July 26, 2017 07:33
@Ladas
Copy link
Contributor Author

Ladas commented Jul 26, 2017

@agrare @pkliczewski now it should be correct :-)

@pkliczewski a side note: I was thinking about removing this branch entirely, since it's slower and we can't apply the batch saving. So the correct solution would be to model every N:M mapping table as a separate InventoryCollection, so getting rid of the HasAndBelongsToMany. So it will stay until we do this in all providers. :-)

@miq-bot
Copy link
Member

miq-bot commented Jul 26, 2017

Checked commits Ladas/manageiq@e384bd6~...0a3faf3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

LGTM

@agrare agrare merged commit 36619c7 into ManageIQ:master Jul 31, 2017
@agrare agrare added this to the Sprint 66 Ending Aug 7, 2017 milestone Jul 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants