WIP: Limit history to 100 entries #1238

MineRobber9000 · 2018-10-02T23:21:52Z

Adds a 100 entry limit to /subjects/history.json requests.

Closes #314

Description:

In this Pull Request we have made the following changes:

Manually add history to subjects.

Note: If anything else needs to happen for this to work, please let me know and I will fix it ASAP.

Adds a 100 entry limit to /subjects/history.json requests. Fixes internetarchive#314.

MineRobber9000 · 2018-10-03T00:30:23Z

Alright, I'll fix these up tommorrow (late night for me) and it should be right as rain. (currently having some issues with getting vagrant working)

mekarpeles · 2018-10-03T01:01:32Z

Happy to help w/ vagrant or docker (at this point docker may be more reliable). Thanks again for contributing :)

openlibrary/plugins/worksearch/subjects.py

mekarpeles

see changes inline, the code doesn't run and has problems which need to be corrected

MineRobber9000 · 2018-10-03T11:49:01Z

My vagrant seems to be crashing with "stack level too deep" errors. (It isn't actually crashing, just hanging and requiring a manual interrupt)

MineRobber9000 · 2018-10-03T13:43:58Z

Invalid gemspec in [/usr/share/rubygems-integration/all/specifications/rbnacl-libsodium.gemspec]: stack level too deep

and then it hangs.

Any idea why? @mekarpeles

Converted limit to int, made 100 a constant, and removed errant argument 'key'.

mekarpeles

Does this run?

mekarpeles · 2018-10-03T20:32:47Z

openlibrary/plugins/worksearch/subjects.py

+    def GET(self):
+        i = web.input()
+	try:
+		limit = int(i.get("limit",self.API_ROWS_LIMIT))


style: space after commas

mekarpeles · 2018-10-03T20:32:59Z

openlibrary/plugins/worksearch/subjects.py

+		limit = int(i.get("limit",self.API_ROWS_LIMIT))
+	except (ValueError, TypeError):
+		return {'error':'limit must be an integer'}
+        if limit>self.API_ROWS_LIMIT:


space before/after >

mekarpeles · 2018-10-03T20:33:11Z

openlibrary/plugins/worksearch/subjects.py

+	try:
+		limit = int(i.get("limit",self.API_ROWS_LIMIT))
+	except (ValueError, TypeError):
+		return {'error':'limit must be an integer'}


space after :

OK! Fixed the other one too!

mekarpeles · 2018-10-03T20:33:26Z

openlibrary/plugins/worksearch/subjects.py

+		return {'error':'limit must be an integer'}
+        if limit>self.API_ROWS_LIMIT:
+            return {'error':'limit must be less than or equal to {}'.format(self.API_ROWS_LIMIT)}
+        return history().get()


Does this work? I think the method is called GET not get

OK, changed to history().GET().

mekarpeles · 2018-10-03T20:33:46Z

openlibrary/plugins/worksearch/subjects.py

+    API_ROWS_LIMIT = 100
+    @jsonapi
+    def GET(self):
+        i = web.input()


this spacing is wrong, does this run?

Could you please be slightly more specific? Where is the spacing wrong? How should I fix it?

MineRobber9000 · 2018-10-04T13:35:02Z

@mekarpeles see above, vagrant is not working for me. I have basically no way to test. I will fix the style comments but I can't test until I can get help and fix Vagrant.

mekarpeles · 2018-10-06T09:35:20Z

re: #1238 (comment)

These instructions will hopefully fix vagrant:
hashicorp/vagrant#9788 (comment)

MineRobber9000 · 2018-10-06T14:52:07Z

While I try to fix vagrant, could you please take a look at the remaining problems with the code and elaborate on why it's wrong? I'm always ready to learn.

mekarpeles · 2018-10-06T16:20:30Z

openlibrary/plugins/worksearch/subjects.py

+    def GET(self):
+        i = web.input()
+	try:
+		limit = int(i.get("limit", self.API_ROWS_LIMIT))


Hi @MineRobber9000, the spacing/indentation is inconsistent on each line such that it wouldn't run.

Please paste the code here before re-submitting:
http://pep8online.com/checkresult

mekarpeles · 2018-10-06T16:22:06Z

MineRobber9000 · 2018-10-06T19:43:05Z

This PR now passes pep8 online aside from line 179. Should I worry about it? (it's 98 chars long, as opposed to the PEP8 standard 79)

mekarpeles · 2018-10-06T19:53:46Z

@MineRobber9000 https://www.python.org/dev/peps/pep-0008/#maximum-line-length

either way, we can't merge this without it being tested and verified on a local development environment. Ideally, we'd want to include a test case that the code works correctly if over 100 records requested.

MineRobber9000 · 2018-10-06T20:13:35Z

vagrant still refuses to work for me. I'd be willing to write up a test case (though it shouldn't be hard) for "does it error correctly when you request too many records" but I cannot test this codebase.

(as an aside, maybe I can get Docker to work.)

mekarpeles · 2018-10-06T20:39:15Z

I've changed the tag of this pull request to WIP (work in progress) as it's not ready to be considered for merge. If you're having trouble getting vagrant or docker up-and-running, can you please upon another issue describing what you're experiencing so we can help you work through it?

As a note: I posted a link to a potential solution to the problem you described above:
#1238 (comment). Docker is our preferred method of setup.

p.s. It would be really helpful if you could spend a bit more independently researching / testing before asking for a review because we have many issues and pull requests requiring attention and several of the issues (e.g. line indentation, no such function existing) could have been discovered by linting + testing (even if vagrant wasn't working) without requiring our assistance. Happy to help if no other resources are available, but if we have to act as a python interpreter for your PR, it's actually less work for us to solve the issue ourselves :). Thanks for understanding and for your continued effort!

mekarpeles · 2018-10-06T20:42:05Z

openlibrary/plugins/worksearch/subjects.py

@@ -176,7 +176,9 @@ def GET(self):
        except (ValueError, TypeError):
            return {'error': 'limit must be an integer'}
        if limit > self.API_ROWS_LIMIT:
-            return {'error': 'limit must be less than or equal to {}'.format(self.API_ROWS_LIMIT)}
+            return {'error':


MineRobber9000 · 2018-10-06T20:45:22Z

Alright, I'm following the docker instructions right now. If all goes well, I'll be able to test this soon.

MineRobber9000 · 2018-10-06T22:59:17Z

Alright, I got the docker images built. How do I go about testing? (do I just try to cause the error to occur?)

(I couldn't find an answer elsewhere)

mekarpeles · 2018-10-08T02:26:02Z

@MineRobber9000 if you visit the endpoint in your browser (localhost:8080 I think it is, once docker is running) you should be able to cause the error by using limit=101

hornc · 2018-10-09T09:33:19Z

Won't this endpoint take precedence over this history endpoint?

openlibrary/openlibrary/plugins/worksearch/subjects.py

Lines 75 to 77 in 878f50e

    
           class subjects_json(delegate.page): 
        
               path = '(/subjects/[^/]+)' 
        
               encoding = "json"

Sorry to be late to the conversation, but now that I've looked into it, /subjects/history.json looks to me like just an example of a particular book subject, rather than a edit history.

mekarpeles · 2018-10-23T19:14:29Z

I think @hornc is right, we should move this issue into @infogami. Thanks everyone for your input!

Limit history to 100 entries

dbde8cd

Adds a 100 entry limit to /subjects/history.json requests. Fixes internetarchive#314.

ParthKolekar reviewed Oct 3, 2018

View reviewed changes

openlibrary/plugins/worksearch/subjects.py Outdated Show resolved Hide resolved

ParthKolekar reviewed Oct 3, 2018

View reviewed changes

openlibrary/plugins/worksearch/subjects.py Outdated Show resolved Hide resolved

ParthKolekar reviewed Oct 3, 2018

View reviewed changes

openlibrary/plugins/worksearch/subjects.py Outdated Show resolved Hide resolved

mekarpeles requested changes Oct 3, 2018

View reviewed changes

Fix some errors

cef4f33

Converted limit to int, made 100 a constant, and removed errant argument 'key'.

mekarpeles requested changes Oct 3, 2018

View reviewed changes

Fix style and incorrect call

96f454b

mekarpeles reviewed Oct 6, 2018

View reviewed changes

Fix indent errors

f6961bb

Fix line length

878f50e

mekarpeles changed the title ~~Limit history to 100 entries~~ WIP: Limit history to 100 entries Oct 6, 2018

mekarpeles reviewed Oct 6, 2018

View reviewed changes

mekarpeles closed this Oct 23, 2018

rohitjain00 mentioned this pull request Mar 23, 2019

Infogami API Endpoints Not Enforcing Sensible Result Limits #314

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Limit history to 100 entries #1238

WIP: Limit history to 100 entries #1238

MineRobber9000 commented Oct 2, 2018

MineRobber9000 commented Oct 3, 2018

mekarpeles commented Oct 3, 2018

mekarpeles left a comment

MineRobber9000 commented Oct 3, 2018

MineRobber9000 commented Oct 3, 2018

mekarpeles left a comment

mekarpeles Oct 3, 2018

MineRobber9000 Oct 4, 2018

mekarpeles Oct 3, 2018

MineRobber9000 Oct 4, 2018

mekarpeles Oct 3, 2018

MineRobber9000 Oct 4, 2018

mekarpeles Oct 3, 2018

MineRobber9000 Oct 4, 2018

mekarpeles Oct 3, 2018

MineRobber9000 Oct 4, 2018 •

edited

Loading

MineRobber9000 commented Oct 4, 2018

mekarpeles commented Oct 6, 2018

MineRobber9000 commented Oct 6, 2018

mekarpeles Oct 6, 2018

mekarpeles commented Oct 6, 2018

MineRobber9000 commented Oct 6, 2018

mekarpeles commented Oct 6, 2018

MineRobber9000 commented Oct 6, 2018

mekarpeles commented Oct 6, 2018

mekarpeles Oct 6, 2018

MineRobber9000 commented Oct 6, 2018

MineRobber9000 commented Oct 6, 2018 •

edited

Loading

mekarpeles commented Oct 8, 2018

hornc commented Oct 9, 2018

mekarpeles commented Oct 23, 2018

WIP: Limit history to 100 entries #1238

WIP: Limit history to 100 entries #1238

Conversation

MineRobber9000 commented Oct 2, 2018

Description:

MineRobber9000 commented Oct 3, 2018

mekarpeles commented Oct 3, 2018

mekarpeles left a comment

Choose a reason for hiding this comment

MineRobber9000 commented Oct 3, 2018

MineRobber9000 commented Oct 3, 2018

mekarpeles 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

MineRobber9000 Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

MineRobber9000 commented Oct 4, 2018

mekarpeles commented Oct 6, 2018

MineRobber9000 commented Oct 6, 2018

Choose a reason for hiding this comment

mekarpeles commented Oct 6, 2018

MineRobber9000 commented Oct 6, 2018

mekarpeles commented Oct 6, 2018

MineRobber9000 commented Oct 6, 2018

mekarpeles commented Oct 6, 2018

Choose a reason for hiding this comment

MineRobber9000 commented Oct 6, 2018

MineRobber9000 commented Oct 6, 2018 • edited Loading

mekarpeles commented Oct 8, 2018

hornc commented Oct 9, 2018

mekarpeles commented Oct 23, 2018

MineRobber9000 Oct 4, 2018 •

edited

Loading

MineRobber9000 commented Oct 6, 2018 •

edited

Loading