Skip to content

Changes update_column to update_attributes in ProjectTreeRestorer#restore_project by using timeless to maintain the current timestamps

Tiago Botelho requested to merge 47765-group-visibility-error-due-to-string-conversion into master

What does this MR do?

From https://gitlab.com/gitlab-org/gitlab-ce/issues/47765#note_81266830 :

@stanhu It appears that this project was created from a template, which is effectively just a project export bundled with GitLab, that we import when someone creates a new project from that template.

The "Create from template" form at https://gitlab.com/projects/new has a few form fields, including visibility_level, that end up in Projects::CreateFromTemplateService as params, are passed along to GitlabProjectsImportService as override_params, stored on project.import_data, read by the asynchronous-running import logic, and applied on the project along with the data from the actual exported JSON using @project.update_columns.

The issue with this is that params from HTTP forms always have string values, even if they are actually meant to be numeric, which means that override_params will look like { "visibility_level" => "0" }. Project#visibility_level= and Project#update_attributes automatically convert this value to an integer so that @project.visibility_level will end up as 0, but Project#update_columns is too low level for that. It will directly update @project.visibility_level without conversion, so it ends up as "0", and will pass the string value along to the DB. The DB is smart enough to perform the conversion itself so that we end up with 0 in the DB, but until the model is reloaded, @project.visibility_level will be "0".

I think we should be using #update_attributes instead of #update_columns in Gitlab::ImportExport::ProjectTreeRestorer#restore_project

@jameslopez What do you think? Do you remember why you used #update_columns in https://gitlab.com/gitlab-org/gitlab-ce/commit/2174e37845f6865c20ba94da5520c8d9e874998f#07953e6bce219868b76e93d653174a723292477a_75_74, and #update before that, rather than #update_attributes?

From https://gitlab.com/gitlab-org/gitlab-ce/issues/47765#note_82226315 :

I can't remember exactly, but probably it has to do with ignoring updated_at (so it preserves old timestamps) and not calling callbacks...

This MR aims to change update_column usage in ProjectTreeRestorer#restore_project by update_attributes with the usage of Gitlab::Timeless to keep timestamps from being updated

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes #47765 (closed)

Edited by Tiago Botelho

Merge request reports