Pre-review ready

Author: alanbato

warehouse/forklift/legacy.py

@@ -759,6 +759,34 @@ def _is_duplicate_file(db_session, filename, hashes):
     return None
 
 
+def _get_release_classifiers(db_session, classifiers_data):
+    """
+    Go over the classifiers of a release, and add any missing ones
+    to the database.
+    """
+
+    # Look up all of the valid classifiers
+    all_classifiers = db_session.query(Classifier).all()
+
+    # Get all the classifiers for this release
+    release_classifiers = [
+        c for c in all_classifiers if c.classifier in classifiers_data
+    ]
+
+    # Determine if we need to add any new classifiers to the database
+    missing_classifiers = set(classifiers_data or []) - set(
+        c.classifier for c in release_classifiers
+    )
+
+    # Add any new classifiers to the database
+    if missing_classifiers:
+        for missing_classifier_name in missing_classifiers:
+            missing_classifier = Classifier(classifier=missing_classifier_name)
+            db_session.add(missing_classifier)
+            release_classifiers.append(missing_classifier)
+    return release_classifiers
+
+
 @view_config(
     route_name="forklift.legacy.file_upload",
     uses_session=True,
@@ -1039,6 +1067,27 @@ def file_upload(request):
             ) from None
 
     canonical_version = packaging.utils.canonicalize_version(form.version.data)
+
+    form_metadata_fields = {
+        # This is a list of all the fields in the form that we
+        # should pull off and insert into our new release.
+        "version",
+        "summary",
+        "license",
+        "author",
+        "author_email",
+        "maintainer",
+        "maintainer_email",
+        "keywords",
+        "platform",
+        "home_page",
+        "download_url",
+        "requires_python",
+    }
+
+    # Determine if this is a draft release or a published one
+    release_is_draft = bool(request.headers.get("Is-Draft", default=False))
+
     try:
         release = (
             request.db.query(Release)
@@ -1060,30 +1109,10 @@ def file_upload(request):
             .one()
         )
     except NoResultFound:
-
-        # Determine if this is a draft release or a published one
-        release_is_draft = bool(request.headers.get("Is-Draft", default=False))
-
-        # Look up all of the valid classifiers
-        all_classifiers = request.db.query(Classifier).all()
-
-        # Get all the classifiers for this release
-        release_classifiers = [
-            c for c in all_classifiers if c.classifier in form.classifiers.data
-        ]
-
-        # Determine if we need to add any new classifiers to the database
-        missing_classifiers = set(form.classifiers.data or []) - set(
-            c.classifier for c in release_classifiers
+        release_classifiers = _get_release_classifiers(
+            request.db, form.classifiers.data
         )
 
-        # Add any new classifiers to the database
-        if missing_classifiers:
-            for missing_classifier_name in missing_classifiers:
-                missing_classifier = Classifier(classifier=missing_classifier_name)
-                request.db.add(missing_classifier)
-                release_classifiers.append(missing_classifier)
-
         release = Release(
             project=project,
             project_name=project.name,
@@ -1110,25 +1139,7 @@ def file_upload(request):
                 html=rendered or "",
                 rendered_by=readme.renderer_version(),
             ),
-            **{
-                k: getattr(form, k).data
-                for k in {
-                    # This is a list of all the fields in the form that we
-                    # should pull off and insert into our new release.
-                    "version",
-                    "summary",
-                    "license",
-                    "author",
-                    "author_email",
-                    "maintainer",
-                    "maintainer_email",
-                    "keywords",
-                    "platform",
-                    "home_page",
-                    "download_url",
-                    "requires_python",
-                }
-            },
+            **{k: getattr(form, k).data for k in form_metadata_fields},
             uploader=request.user,
             uploaded_via=request.user_agent,
             published=None if release_is_draft else datetime.now(tz=timezone.utc),
@@ -1146,17 +1157,48 @@ def file_upload(request):
                 submitted_from=request.remote_addr,
             )
         )
-
         project.record_event(
             tag="project:release:add",
             ip_address=request.remote_addr,
             additional={
                 "submitted_by": request.user.username,
                 "canonical_version": release.canonical_version,
-                "version_or_draft": release.version_or_draft,
             },
         )
-
+    else:
+        # An existing release was found. Update its metadata if it's a draft.
+        if release.is_draft:
+            release_classifiers = _get_release_classifiers(
+                request.db, form.classifiers.data
+            )
+            for rc in release_classifiers:
+                release._classifiers = release_classifiers
+            new_dependencies = list(
+                _construct_dependencies(
+                    form,
+                    {
+                        "requires": DependencyKind.requires,
+                        "provides": DependencyKind.provides,
+                        "obsoletes": DependencyKind.obsoletes,
+                        "requires_dist": DependencyKind.requires_dist,
+                        "provides_dist": DependencyKind.provides_dist,
+                        "obsoletes_dist": DependencyKind.obsoletes_dist,
+                        "requires_external": DependencyKind.requires_external,
+                        "project_urls": DependencyKind.project_url,
+                    },
+                )
+            )
+            if new_dependencies:
+                release.dependencies = new_dependencies
+            if form.description.data:
+                release.description.content_type = description_content_type
+                release.description.raw = form.description.data or ""
+                release.description.html = rendered or ""
+                release.description.rendered_by = readme.renderer_version()
+            for field in form_metadata_fields:
+                field_value = getattr(form, field).data
+                if field_value:
+                    setattr(release, field, field_value)
     # TODO: We need a better solution to this than to just do it inline inside
     #       this method. Ideally the version field would just be sortable, but
     #       at least this should be some sort of hook or trigger.
@@ -1274,8 +1316,15 @@ def file_upload(request):
                 "The digest supplied does not match a digest calculated "
                 "from the uploaded file.",
             )
-        # Skip duplicate check for files when it's a draft release
-        if not release.is_draft:
+        # Skip duplicate check for files when it's a draft release,
+        # and delete existing files instead
+        if release.is_draft:
+            existing_file = (
+                request.db.query(File).filter(File.filename == filename).first()
+            )
+            if existing_file is not None:
+                request.db.delete(existing_file)
+        else:
             # Check to see if the file that was uploaded exists already or not.
             is_duplicate = _is_duplicate_file(request.db, filename, file_hashes)
             if is_duplicate:
@@ -1363,7 +1412,17 @@ def file_upload(request):
         # TODO: This should be handled by some sort of database trigger or a
         #       SQLAlchemy hook or the like instead of doing it inline in this
         #       view.
-        request.db.add(Filename(filename=filename))
+        #
+        # If this is a draft release and the filename is
+        # already on the registry, do nothing.
+        if release.is_draft:
+            if (
+                request.db.query(Filename).filter(Filename.filename == filename).first()
+                is None
+            ):
+                request.db.add(Filename(filename=filename))
+        else:
+            request.db.add(Filename(filename=filename))
 
         # Store the information about the file in the database.
         file_ = File(

warehouse/manage/views.py

@@ -1534,7 +1534,7 @@ def _error(message):
             self.request.route_path(
                 "manage.project.release",
                 project_name=self.release.project.name,
-                version=self.release.version,
+                version=self.release.version_or_draft,
             )
         )
 

warehouse/migrations/versions/93388f06f5e7_draft_releases.py

@@ -0,0 +1,77 @@
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""
+Draft Releases
+
+Revision ID: 93388f06f5e7
+Revises: 80018e46c5a4
+Create Date: 2020-09-29 18:23:35.798571
+"""
+
+from alembic import op
+import sqlalchemy as sa
+
+
+revision = "93388f06f5e7"
+down_revision = "80018e46c5a4"
+
+# Note: It is VERY important to ensure that a migration does not lock for a
+#       long period of time and to ensure that each individual migration does
+#       not break compatibility with the *previous* version of the code base.
+#       This is because the migrations will be ran automatically as part of the
+#       deployment process, but while the previous version of the code is still
+#       up and running. Thus backwards incompatible changes must be broken up
+#       over multiple migrations inside of multiple pull requests in order to
+#       phase them in over multiple deploys.
+
+
+def upgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.add_column("releases", sa.Column("project_name", sa.Text(), nullable=True))
+    op.add_column("releases", sa.Column("published", sa.DateTime(), nullable=True))
+    # ### end Alembic commands ###
+    # Fill the project_name and published columns
+    op.execute(
+        """
+        UPDATE releases AS target SET published = (
+        SELECT created
+        FROM releases AS source WHERE source.id = target.id
+        );
+    """
+    )
+    op.execute(
+        """
+        UPDATE releases AS r SET project_name = (
+            SELECT name
+            FROM projects AS p WHERE p.id = r.project_id
+        )
+    """
+    )
+    # Create the hashing database function for our draft identifiers
+    op.execute(
+        """
+        CREATE FUNCTION make_draft_hash(project_name text, version text) returns text as $$
+                SELECT  md5(project_name || version)
+            $$
+            LANGUAGE SQL
+            IMMUTABLE
+            RETURNS NULL ON NULL INPUT;
+    """
+    )
+
+
+def downgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.drop_column("releases", "published")
+    op.drop_column("releases", "project_name")
+    # ### end Alembic commands ###
+    op.execute("DROP FUNCTION make_draft_hash")

warehouse/templates/manage/history.html

@@ -31,7 +31,7 @@ <h2>{% trans %}Security history{% endtrans %}</h2>
       {% trans %}Created by:{% endtrans %} <a href="{{ request.route_path('accounts.profile', username=event.additional.created_by) }}">{{ event.additional.created_by }}</a>
     </small>
     {% elif event.tag == "project:release:add" %}
-    <strong>{% trans href=request.route_path('manage.project.release', project_name=project.name, version=event.additional.version_or_draft), version=event.additional.canonical_version %}<a href="{{ href }}">Release version {{ version }}</a> created{% endtrans %}</strong><br>
+    <strong>{% trans href=request.route_path('manage.project.release', project_name=project.name, version=event.additional.canonical_version), version=event.additional.canonical_version %}<a href="{{ href }}">Release version {{ version }}</a> created{% endtrans %}</strong><br>
     <small>
       {% trans %}Added by:{% endtrans %} <a href="{{ request.route_path('accounts.profile', username=event.additional.submitted_by) }}">{{ event.additional.submitted_by }}</a>
     </small>