Skip to content

Creates models for the DB - #13

Merged
kartiksibal merged 12 commits into
developfrom
models_db
Jul 5, 2017
Merged

Creates models for the DB#13
kartiksibal merged 12 commits into
developfrom
models_db

Conversation

@kartiksibal

Copy link
Copy Markdown
Contributor

Signed-off-by: Kartik Sibal kartiksibal@gmail.com

kartiksibal and others added 12 commits June 19, 2017 00:22
Signed-off-by: Thomas Druez <tdruez@nexb.com>
Signed-off-by:Kartik Sibal <kartiksibal@gmail.com>
Signed-off-by: Kartik Sibal <kartiksibal@gmail.com>
Signed-off-by: Kartik Sibal <kartiksibal@gmail.com>
Signed-off-by: Kartik Sibal <kartiksibal@gmail.com>
@kartiksibal
kartiksibal merged commit f74520e into develop Jul 5, 2017

@pombredanne pombredanne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here is some feedback.... next time it would be best if we can review this kind of foundations in a PR before merging.



class VulnerabilityReference(models.Model):
vulnerability_id = models.ForeignKey('Vulnerability')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

when you use a FK here, do not use _id. What you get is an object, not an ID. Also reference the model as a class here, not as a string. This applies to all the models here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@pombredanne I read it here: https://docs.djangoproject.com/en/1.11/ref/models/fields/#django.db.models.ForeignKey

Could you please explain a bit, as to why in the documentation it is referenced as a string?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

https://docs.djangoproject.com/en/1.11/ref/models/fields/#database-representation

Behind the scenes, Django appends "_id" to the field name to create its database column name. In the above example, the database table for the Car model will have a manufacturer_id column. (You can change this explicitly by specifying db_column) However, your code should never have to deal with the database column name, unless you write custom SQL. You’ll always deal with the field names of your model object.

Simple use: vulnerability = models.ForeignKey('Vulnerability')

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@tdruez, Could you maybe explain @pombredanne ' point of view?

vulnerability_id = models.ForeignKey('Vulnerability')
source = models.CharField(max_length=50)
reference_id = models.CharField(max_length=50)
url = models.URLField(max_length=50)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When did you last see a URL that's 50 char long?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@pombredanne I didn't understand your tone here. Too long or short? 😛

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

50 is way to short, 1024 should be plenty.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@tdruez Right, same thought. 😛


class VulnerabilityReference(models.Model):
vulnerability_id = models.ForeignKey('Vulnerability')
source = models.CharField(max_length=50)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add a help to each field that documents them.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pombredanne is referring to https://docs.djangoproject.com/en/1.11/ref/models/fields/#help-text
This is a great way to document the purpose of each field. This will also be re-used in the admin and API documentation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@tdruez This is great. 👍



class Package(models.Model):
package_id = models.AutoField(primary_key=True)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not needed. We can use the implicit id instead.
We also need some unique together on (platform, name, version)

# Create your models here.

class Vulnerability(models.Model):
vulnerability_id = models.AutoField(primary_key=True)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if we are going to need a vulnerability ID that's not an auto field I think.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@pombredanne I had the same dilemma, my thought process was to primarily diff. package id from vulnerability id.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can remove this line. See https://docs.djangoproject.com/en/1.11/ref/models/fields/#primary-key

If you don’t specify primary_key=True for any field in your model, Django will automatically add an AutoField to hold the primary key

This apply to all models.

@kartiksibal kartiksibal Jul 7, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@tdruez I did read that. My primary motive was to diff. vulnerability id from package id. Henc, I didn't use the default id. But, I'll change that. 👍

package_id = models.AutoField(primary_key=True)
platform = models.CharField(max_length=50)
name = models.CharField(max_length=50)
version = models.FloatField(max_length=50)

@tdruez tdruez Jul 7, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should use a CharField for all version type fields.
FloatField is not suitable for something like "1.2.3"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@tdruez Pro Tip 👍 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants