[Prefix] DB changes for Prefix reservation #4394

shishirx34 · 2017-07-13T23:20:42Z

Adding tables and relevant columns.

joelverhagen · 2017-07-14T00:12:06Z

src/NuGetGallery.Core/Entities/ReservedPrefix.cs

+        public ReservedPrefix(string pattern, bool isPublicNamespace)
+        {
+            Pattern = pattern;
+            IsPublicNamespace = isPublicNamespace;


This means that the namespace is opened up by the owner to allow everyone to push?

joelverhagen · 2017-07-14T00:13:07Z

src/NuGetGallery.Core/Entities/ReservedPrefix.cs

+
+        [StringLength(CoreConstants.MaxPackageIdLength)]
+        [Required]
+        public string Pattern { get; set; }


We talked about an exact match vs. prefix match. Does this somehow cover both?

Had offline talk with Joel. Changing this to "Value" and adding a new column "IsExactMatch" for cases of "jquery" vs "jquery*" patterns

skofman1 · 2017-07-14T00:23:43Z

src/NuGetGallery.Core/Entities/PackageRegistration.cs

        public virtual ICollection<User> Owners { get; set; }
        public virtual ICollection<Package> Packages { get; set; }
+        public virtual ICollection<ReservedPrefix> ReservedPrefixes { get; set; }


in what scenario will we need to map PackageRegistration to ReserverPrefixes?

one of the scenario, lets say we want to delete a reserved prefix and it matches an ID, we want to find which other prefixes does this ID match to determine if the "Verified" flag needs to be removed or not? I think it will be useful in the future as well to show all the prefixes an ID matches, in account page may be.

For the account page scenario will not that information come from the User information?
What is the delete flow and what are the delete scenarios?

There are not any tests in this PR.

That would be for users to find which prefixes they own. But for a given package which all prefix it matches would come from this list. Its possible you are a co-owner on a package which matches a prefix owned by someone else so it won't show up in your account page. I am just spitballing here, but you see the point of keeping this mapping of package ids to prefixes, right?

The delete flow would be when a prefix is deleted we want to remove the "verified" tag on a package reservation only if there are no other reserved prefixes matching this package ID owned by one of the owners of this package.

I am planning to add tests with the next PR for adding the admin flow of adding/removing prefixes. That way we will have complete picture. I will look into it and if possible add a few tests in here as well.

How do you map a package to a prefix?
Is Microsoft.Asp.Bla is under Microsoft., Microsoft.Asp., both?

Its under both. When you allocate a reserved prefix to a user, we will check all the packages owned by that user and check if that package matches this reserved prefix. If yes, we add the mapping to it.

joelverhagen · 2017-07-14T17:38:06Z

src/NuGetGallery.Core/Entities/ReservedPrefix.cs

+{
+    public class ReservedPrefix : IEntity
+    {
+        public ReservedPrefix() : this(null, false, false)


nit: label null and bool params

skofman1 · 2017-07-14T19:45:26Z

src/NuGetGallery.Core/Entities/ReservedPrefix.cs

+
+        public bool IsExactMatch { get; set; }
+
+        public virtual ICollection<PackageRegistration> PackageRegistrations { get; set; }


I'm concerned about unnecessary joins and loading a lot of not needed info. For example, a prefix can have multiple package registrations. Not every time I load a prefix I need those registrations (that can be in the hundreds). If you write the code that loads ReservedPrefix carefully, this can be avoided. But you should be aware.

Sure, I will keep it in mind and optimize it.

It looks like these related entities would be lazy loaded when the property is accessed. (See Requirements for Creating POCO Proxies.) If not, please consider enabling lazy loading.

skofman1 · 2017-07-14T19:49:31Z

src/NuGetGallery/Migrations/201707131923337_PrefixReservation.cs

@@ -0,0 +1,68 @@
+namespace NuGetGallery.Migrations
+{


skofman1 · 2017-07-14T19:53:29Z

src/NuGetGallery/Migrations/201707131923337_PrefixReservation.cs

+                .Index(t => t.User_Key);
+
+            AddColumn("dbo.Users", "Verified", c => c.Boolean(nullable: false));
+            AddColumn("dbo.PackageRegistrations", "Verified", c => c.Boolean(nullable: false));


Do we need to have "Verified" explicitly on PackageRegistrations table, or perhaps simply finding the registration has a prefix in dbo.PackageRegistrationReservedPrefixes is enough?

I think having it on the PR table is better. We don't need to do additional lookups to determine if a visual indicator is necessary, this makes it easier for auxiliary job too. I would keep it on this table.

You are introducing complexity by forcing two tables to be consistent. For example, when updating a package with a prefix you are going to require a transaction. IMO, it's better to keep it simple, and have the correct indices for a quick join.

Had an offline talk with Svetlana. We are in agreement that it would be beneficial to have this property on PackageRegistrations given that this would be set most cases only once plus not doing this would involve making joins on every query to the package details page(high frequency). Leaving this in.

chenriksson · 2017-07-17T19:39:55Z

src/NuGetGallery.Core/Entities/ReservedPrefix.cs

+
+        public bool IsPublicNamespace { get; set; }
+
+        public bool IsExactMatch { get; set; }


IsExactMatch is when the 'ReservedPrefix' isn't a prefix? :-)

Maybe the entity should be ReservedNamespace, with column IsPrefix? I would probably also consider renaming IsPublicNamespace to SharedNamespace to indicate that it was shared by the original owner.

Yes that is what it is.

I am fine with those names as well.

chenriksson · 2017-07-17T19:41:47Z

src/NuGetGallery.Core/Entities/User.cs

        public bool EmailAllowed { get; set; }

+        public bool Verified { get; set; }


As mentioned before, I would consider removing User.Verified. I don't think it makes sense on the User. It's really a package state when the package ID matches a reserved namespace, right?

dtivel · 2017-07-14T20:18:22Z

src/NuGetGallery.Core/Entities/PackageRegistration.cs

        }

        [StringLength(CoreConstants.MaxPackageIdLength)]
        [Required]
        public string Id { get; set; }

        public int DownloadCount { get; set; }
+
+        public bool Verified { get; set; }


IsVerified

done

dtivel · 2017-07-17T20:57:31Z

src/NuGetGallery.Core/Entities/ReservedPrefix.cs

+
+        public bool IsExactMatch { get; set; }
+
+        public virtual ICollection<PackageRegistration> PackageRegistrations { get; set; }


It looks like these related entities would be lazy loaded when the property is accessed. (See Requirements for Creating POCO Proxies.) If not, please consider enabling lazy loading.

shishirx34 added 4 commits July 13, 2017 15:10

DB changes for prefix reservation

b22e721

nit fix

6383c86

refactor

2915d3e

format

860db91

shishirx34 requested review from joelverhagen, agr, xavierdecoster, ryuyu, chenriksson, cristinamanum, skofman1 and scottbommarito July 13, 2017 23:20

dnfclas added the cla-already-signed label Jul 13, 2017

joelverhagen reviewed Jul 14, 2017

View reviewed changes

skofman1 reviewed Jul 14, 2017

View reviewed changes

address feedback

6a3b89d

joelverhagen reviewed Jul 14, 2017

View reviewed changes

add params

5338adb

joelverhagen approved these changes Jul 14, 2017

View reviewed changes

skofman1 reviewed Jul 14, 2017

View reviewed changes

src/NuGetGallery/Migrations/201707131923337_PrefixReservation.cs

@@ -0,0 +1,68 @@

namespace NuGetGallery.Migrations

{

Copy link

Contributor

skofman1 Jul 14, 2017

Choose a reason for hiding this comment

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

header

skofman1 reviewed Jul 14, 2017

View reviewed changes

add header

4d2326f

shishirx34 requested a review from dtivel July 17, 2017 17:31

chenriksson suggested changes Jul 17, 2017

View reviewed changes

dtivel approved these changes Jul 17, 2017

View reviewed changes

address feedback

4aeac85

chenriksson approved these changes Jul 19, 2017

View reviewed changes

shishirx34 merged commit 78b7d77 into feature-prefix Jul 19, 2017

shishirx34 deleted the prefix-db-changes branch September 1, 2017 00:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Prefix] DB changes for Prefix reservation #4394

[Prefix] DB changes for Prefix reservation #4394

shishirx34 commented Jul 13, 2017

joelverhagen Jul 14, 2017

shishirx34 Jul 14, 2017

joelverhagen Jul 14, 2017

shishirx34 Jul 14, 2017

skofman1 Jul 14, 2017

shishirx34 Jul 14, 2017

cristinamanum Jul 14, 2017

shishirx34 Jul 14, 2017

skofman1 Jul 14, 2017

shishirx34 Jul 14, 2017

joelverhagen Jul 14, 2017

skofman1 Jul 14, 2017

shishirx34 Jul 14, 2017

dtivel Jul 17, 2017

skofman1 Jul 14, 2017

skofman1 Jul 14, 2017

shishirx34 Jul 14, 2017

skofman1 Jul 14, 2017

shishirx34 Jul 17, 2017

chenriksson Jul 17, 2017

shishirx34 Jul 19, 2017

chenriksson Jul 17, 2017

dtivel Jul 14, 2017 •

edited by shishirx34

Loading

dtivel Jul 17, 2017


		public bool IsExactMatch { get; set; }

		public virtual ICollection<PackageRegistration> PackageRegistrations { get; set; }


		public bool IsPublicNamespace { get; set; }

		public bool IsExactMatch { get; set; }

		public bool EmailAllowed { get; set; }

		public bool Verified { get; set; }

[Prefix] DB changes for Prefix reservation #4394

[Prefix] DB changes for Prefix reservation #4394

Conversation

shishirx34 commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtivel Jul 14, 2017 • edited by shishirx34 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtivel Jul 14, 2017 •

edited by shishirx34

Loading