Add modeling for intersphinx data #5289

ericholscher · 2019-02-13T19:05:44Z

This adds modeling for Intersphinx data. It will be automatically updated on build, so that we have it indexed. The goal here will be to add this to search results, but I broke up the PR's to make them easier to review and merge.

Closes #4767

This removes the second entry point to site search, and unifies all our searching to use fancy facets and be nice.

safwanrahman · 2019-02-15T17:26:06Z

I was thinking about relating the domain data with HTMLFile object. @ericholscher what do you think?

readthedocs/domaindata/models.py

stsewd · 2019-02-18T15:44:07Z

readthedocs/domaindata/models.py

+    )
+    version = models.ForeignKey(Version, verbose_name=_('Version'),
+                                related_name='domain_data')
+    modified_date = models.DateTimeField(_('Publication date'), auto_now=True)


We could start using this standard #4864

stsewd · 2019-02-18T15:46:43Z

readthedocs/domaindata/models.py

+    version = models.ForeignKey(Version, verbose_name=_('Version'),
+                                related_name='domain_data')
+    modified_date = models.DateTimeField(_('Publication date'), auto_now=True)
+    commit = models.CharField(_('Commit'), max_length=255)


Do we really need this? I think we already have this from the version 🤔

Yes, a file can be removed from a specific commit, and we need to track this to delete it.

stsewd · 2019-02-18T15:58:35Z

readthedocs/domaindata/models.py

+            '''
+
+    @property
+    def doc_type(self):


Not sure if this is the correct name, I wasn't able to find something that could be used instead. I mean, this is just the full object not, the type of document.

This is more like a "role name" or "directive name" in the Sphinx language as I can read from their docs. Considering that this is used as .. py:function:: or py:function:'spam'.

Maybe "role_name" is a better name.

readthedocs/projects/tasks.py

stsewd · 2019-02-18T16:11:40Z

readthedocs/projects/tasks.py

+        for name, einfo in sorted(invdata[key].items()):
+            url = einfo[2]
+            if '#' in url:
+                doc_name, anchor = url.split('#')


We could use urlparse https://docs.python.org/3/library/urllib.parse.html, but the logic here is very simple, so not really sure.

Yea, feels unnecessary. It's also not a full URL, so it might be confused by it.

ericholscher · 2019-02-20T14:07:31Z

I was thinking about relating the domain data with HTMLFile object. @ericholscher what do you think?

@safwanrahman Can you say a bit more about this? They aren't representing a file that we have on disk, so I don't think it's a good match. It could be useful to optionally have them point to an HTMLFile on the model, but I'm not sure what the value would be?

humitos

Looks good and I think this has potential. Although, I'm not 100% sure how or where you are thinking to use this.

I left some Python style comments that I think can be considered.

As I understand this PR, we are converting the objects.inv into data on our db. If I'm correct, won't this be a huge amount of data going to our db that could potentially cause an issue?

humitos · 2019-02-21T11:04:39Z

readthedocs/domaindata/admin.py

+from .models import DomainData
+
+
+class DomainDataAdmin(admin.ModelAdmin):


DomainData name conflicts with our Domain model to me.

Considering that we are talking about Sphinx Domains here, I'd name all of these ones (including the Django App) as SphinxDomain. I think it will be better to get the meaning immediately when re reading about this.

humitos · 2019-02-21T11:06:51Z

readthedocs/domaindata/models.py

+
+    project = models.ForeignKey(
+        Project,
+        related_name='domain_data',


Same here, we will have project.domains and project.domain_data. I think it will be clearer as project.domains (current relationship) and project.sphinx_domains (or project.sphinxdomains), IMO.

humitos · 2019-02-21T11:08:35Z

readthedocs/domaindata/models.py

+    def __str__(self):
+        return f'''
+            DomainData [{self.project.slug}:{self.version.slug}]
+            [{self.domain}:{self.type}] {self.name} -> {self.doc_name}#{self.anchor}


This should probably be wrapped.

humitos · 2019-02-21T11:18:27Z

readthedocs/domaindata/models.py

+            '''
+
+    @property
+    def doc_type(self):


This is more like a "role name" or "directive name" in the Sphinx language as I can read from their docs. Considering that this is used as .. py:function:: or py:function:'spam'.

Maybe "role_name" is a better name.

humitos · 2019-02-21T11:19:39Z

readthedocs/domaindata/models.py

+        return f'{self.domain}:{self.type}'
+
+    @property
+    def doc_url(self):


I'd name it as docs_url to follow the same pattern that we have in other places.

humitos · 2019-02-21T11:25:17Z

readthedocs/projects/tasks.py

+    invdata = intersphinx.fetch_inventory(MockApp(), '', object_file)
+    for key in sorted(invdata or {}):
+        domain, _type = key.split(':')
+        for name, einfo in sorted(invdata[key].items()):


Isn't it clearer to use .items() in the first loop here?

for key, value in invdata.items(): domain, _type = key.split(':') for name, einfo in value:

humitos · 2019-02-21T11:27:34Z

readthedocs/projects/tasks.py

+            log.warning('Sphinx MockApp: %s', msg)
+
+    invdata = intersphinx.fetch_inventory(MockApp(), '', object_file)
+    for key in sorted(invdata or {}):


Why these two loops need to be sorted?

humitos · 2019-02-21T11:32:34Z

readthedocs/projects/tasks.py

+    for key in sorted(invdata or {}):
+        domain, _type = key.split(':')
+        for name, einfo in sorted(invdata[key].items()):
+            url = einfo[2]


I'd add a small comment with previous to this line, with the expected structure of this einfo object. Like

# (key, name, url, doc type, etc) # ('py', 'function', '/some-nice-url/python-function/', 'func', ...)

or... a link to the docs.

I found debugging this complicated later.

humitos · 2019-02-21T11:35:38Z

readthedocs/projects/tasks.py

+            else:
+                doc_name, anchor = url, ''
+            display_name = einfo[3]
+            obj, _ = DomainData.objects.get_or_create(


I think this will fail the first time because commit is not passed here and it's not marked as null=True.

humitos · 2019-02-21T11:39:34Z

readthedocs/domaindata/api.py

+            'name',
+            'display_name',
+            'doc_type',
+            'doc_url',


I suppose this is the most important field we want for the use case you mentioned, as we will want to embed the section when hovering with the mouse or something like that, right?

safwanrahman · 2019-02-22T08:53:47Z

I was thinking about relating the domain data with HTMLFile object. @ericholscher what do you think?

@safwanrahman Can you say a bit more about this? They aren't representing a file that we have on disk, so I don't think it's a good match. It could be useful to optionally have them point to an HTMLFile on the model, but I'm not sure what the value would be?

@ericholscher If each of the domain data could pointed to a HTMLFile object, we can index the HTMLFile and DomainData object together in Elasticsearch. So while searching, we can show results based on the page and the section.
In current situation, as there are no relation among them, we need to index them separately and its not possible to show a user about the subsection in search results.

humitos · 2019-02-25T10:15:57Z

readthedocs/rtd_tests/tests/test_privacy_urls.py

@@ -364,7 +364,7 @@ def setUp(self):
            'api_webhook_gitlab': {'status_code': 405},
            'api_webhook_bitbucket': {'status_code': 405},
            'api_webhook_generic': {'status_code': 403},
-            'domaindata-detail': {'status_code': 404},
+            'sphinx_domains-detail': {'status_code': 404},


I think it doesn't matter too much, but to keep consistency here with the other endpoints, I'd say that it should be called sphinxdomains-detail (without the _).

This is autogenerated by REST framework.

Well, yes... but it comes from the register: https://github.com/rtfd/readthedocs.org/pull/5289/files/021e12d9472f3e4cb1f7f69413e0af0e25808c86..ab7a30cf1cb40012ce71704824723aa5c1597b67#diff-48d1599aaf2a6be451a9f14bdf0cd81aR38

I should have commented that other line instead :)

Agreed. It also shouldn't be plural. Fixed.

ericholscher · 2019-02-26T18:31:52Z

Would love to get this merged for deploy this week as well, if folks have time to review.

readthedocs/projects/tasks.py

humitos

It's ready to merge to me.

Just wanted to re-raise my comment about consistency on the DRF URLs. I'm not blocking on that, anyway but I think it worth to change it if possible.

humitos · 2019-02-27T09:37:05Z

readthedocs/rtd_tests/tests/test_privacy_urls.py

@@ -364,7 +364,7 @@ def setUp(self):
            'api_webhook_gitlab': {'status_code': 405},
            'api_webhook_bitbucket': {'status_code': 405},
            'api_webhook_generic': {'status_code': 403},
-            'domaindata-detail': {'status_code': 404},
+            'sphinx_domains-detail': {'status_code': 404},


Well, yes... but it comes from the register: https://github.com/rtfd/readthedocs.org/pull/5289/files/021e12d9472f3e4cb1f7f69413e0af0e25808c86..ab7a30cf1cb40012ce71704824723aa5c1597b67#diff-48d1599aaf2a6be451a9f14bdf0cd81aR38

I should have commented that other line instead :)

Co-Authored-By: ericholscher <[email protected]>

ericholscher added 21 commits October 23, 2018 15:23

Remove old test references to intersphinx

83c6f40

Initial commit of domains app

db5fbd8

Merge branch 'allow-local-files-in-dev' into intersphinx-modeling

c2cd164

Add initial domaindata modeling and API integration

41b2857

Merge remote-tracking branch 'origin/master' into intersphinx-modeling

806d3c2

Add indexing of DomainData to the builds.

0f82c26

Merge remote-tracking branch 'origin/master' into intersphinx-modeling

639b219

Merge branch 'readd-search-signals' into intersphinx-modeling

67d909a

Cleanup of some domain stuff

537c66a

Add search to Domains 🎉

647ae3f

Merge branch 'readd-search-signals' into intersphinx-modeling

5b7d599

Add initial search implementation to DomainData

1dfabe8

Move project search to a subset of site search

08296ae

This removes the second entry point to site search, and unifies all our searching to use fancy facets and be nice.

Small cleanup around faceting

1e087fb

Limit resuts by DomainData type

12bdf04

Merge remote-tracking branch 'origin/master' into intersphinx-modeling

8e015c9

Remove old domains app

444e739

Remove search changes

1672421

Missed a few

462545e

Missed a bit more

f53e589

remove random bits that got merged in

98aae46

ericholscher requested a review from a team February 13, 2019 19:05

ericholscher added 2 commits February 14, 2019 13:36

Undo url fix

c934046

Fix linting issues

96fa45a

stsewd reviewed Feb 18, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into intersphinx-modeling

d8d75d5

ericholscher requested a review from a team February 20, 2019 19:40

Fix tests and linting

09b3c80

ericholscher added 4 commits February 20, 2019 16:45

Review feedback

0da9283

Fix lint

1bc31d0

Move sphinx to a runtime requirement.

97851c4

Use timestamped model for standardized class names

021e12d

humitos reviewed Feb 21, 2019

View reviewed changes

Address review feedback

ab7a30c

humitos reviewed Feb 25, 2019

View reviewed changes

ericholscher requested review from humitos, stsewd and a team and removed request for humitos February 26, 2019 18:07

stsewd previously approved these changes Feb 26, 2019

View reviewed changes

readthedocs/projects/tasks.py Outdated Show resolved Hide resolved

humitos previously approved these changes Feb 27, 2019

View reviewed changes

ericholscher added 2 commits February 27, 2019 09:50

Merge remote-tracking branch 'origin/master' into intersphinx-modeling

fa72754

Fix API name

4ea9121

ericholscher dismissed stale reviews from humitos and stsewd via 4ea9121 February 27, 2019 12:50

ericholscher and others added 2 commits February 27, 2019 09:51

Fix test too

ec71993

Update readthedocs/projects/tasks.py

18877c1

Co-Authored-By: ericholscher <[email protected]>

stsewd approved these changes Feb 27, 2019

View reviewed changes

ericholscher merged commit 1fd489c into master Feb 27, 2019

delete-merged-branch bot deleted the intersphinx-modeling branch February 27, 2019 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add modeling for intersphinx data #5289

Add modeling for intersphinx data #5289

ericholscher commented Feb 13, 2019 •

edited

Loading

safwanrahman commented Feb 15, 2019

stsewd Feb 18, 2019

stsewd Feb 18, 2019

ericholscher Feb 20, 2019

stsewd Feb 18, 2019

humitos Feb 21, 2019

stsewd Feb 18, 2019

ericholscher Feb 20, 2019

ericholscher commented Feb 20, 2019 •

edited

Loading

humitos left a comment

humitos Feb 21, 2019

humitos Feb 21, 2019

humitos Feb 21, 2019

humitos Feb 21, 2019

humitos Feb 21, 2019

humitos Feb 21, 2019

humitos Feb 21, 2019

humitos Feb 21, 2019

humitos Feb 21, 2019

humitos Feb 21, 2019

safwanrahman commented Feb 22, 2019

humitos Feb 25, 2019

ericholscher Feb 25, 2019

humitos Feb 27, 2019

ericholscher Feb 27, 2019

ericholscher commented Feb 26, 2019

humitos left a comment

humitos Feb 27, 2019

		from .models import DomainData


		class DomainDataAdmin(admin.ModelAdmin):

Add modeling for intersphinx data #5289

Add modeling for intersphinx data #5289

Conversation

ericholscher commented Feb 13, 2019 • edited Loading

safwanrahman commented Feb 15, 2019

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

ericholscher commented Feb 20, 2019 • edited Loading

humitos left a 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

safwanrahman commented Feb 22, 2019

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

ericholscher commented Feb 26, 2019

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher commented Feb 13, 2019 •

edited

Loading

ericholscher commented Feb 20, 2019 •

edited

Loading