Fix for scrapinghub#116 #139

adityas114 · 2020-05-26T12:45:52Z

Fixes #116. I fixed the order in rdfa.py by checking the correct order in the document object. I also fixed test 'test_rdfa_not_preserving_order'.

I fix the order in the json-ld string returned by rdflib by checking the correct order in the document object.

The test case 'test_rdfa_not_preserving_order' should not call extruct.extract for only RDFa, but for all formats, since the expected json string also has all formats.

Removed 'xfail' tag from 'tagtest_rdfa_not_preserving_order' test case as issue scrapinghub#116 is fixed.

I fix the order in the json-ld string returned by rdflib by checking the correct order in the document object.

codecov · 2020-05-26T12:47:29Z

Codecov Report

Merging #139 into master will increase coverage by 1.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #139      +/-   ##
==========================================
+ Coverage   88.06%   89.15%   +1.09%     
==========================================
  Files          11       12       +1     
  Lines         486      535      +49     
  Branches      108      121      +13     
==========================================
+ Hits          428      477      +49     
  Misses         52       52              
  Partials        6        6

Impacted Files	Coverage Δ
extruct/rdfa.py	`100.00% <100.00%> (ø)`
extruct/__init__.py	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a64ce58...78c76a3. Read the comment docs.

ivanprado · 2020-05-27T14:17:29Z

Thank you @adityas114 for your contribution! I'll have a look in the following days. I already have a quick look and have some comments:

I wonder if the approach is the correct. I mean, the proposed code is patching somehow the result from the source library (RDFlib or pyRDFa, not sure. See RDFa ordering not preserved on duplicated properties #116 (comment)). A better fix would be to fix the original library. But also seems that extucts pinned the rdflib version probably because newer versions introduces some incompatibilities. So then maybe patching is the right thing to do. Not sure. I'll see the code during the next days and probably I'll have it clearer.
I see that the code is iterating several times over the tree. I wonder how this is impacting the performance. Could you measure the impact on the performance of your changes? Also, wonder if speed could be improved if you use xpath expressions in some places instead of transversing the tree.
There are some parts of your code that remain untested. See the codecov report: https://codecov.io/gh/scrapinghub/extruct/pull/139/diff?src=pr&el=tree#diff-ZXh0cnVjdC9yZGZhLnB5. Tests covering them will be required.

lopuhin · 2020-05-27T14:26:30Z

But also seems that extucts pinned the rdflib version probably because newer versions introduces some incompatibilities. So then maybe patching is the right thing to do.

#135 is also related

adityas114 · 2020-05-27T17:53:10Z

Thank you @ivanprado for you comments.

I agree that ideally it should be fixed in the original library, but I believe that maybe we can patch it in extruct itself for the time being.
Yes xpath may improve performance, but I am not familiar with it so I didn't use it for now. I'll look into it.
Okay I'll add more tests where required.

Let me know if there is any more feedback 😄

Opengraph output is not correct when I updated the 'elysianfields.html' file.

Updated to test issue scrapinghub#116

Optimised performance of 'fixOrder' function by using xpath.

Fixed minor bugs

adityas114 · 2020-05-27T21:12:55Z

@ivanprado

I have added tests to improve code coverage, and I have used xpath to improve efficiency.

For testing, I edited 'elysianfields.html', however after changing two other tests fail. From what I understand, it seems that there is an issue with the implementation of open graph protocol. I have added xfail tags for those 2 tests.

Let me know what you think 😄

extruct/rdfa.py

ivanprado

Thanks @adityas114 for the changes! After a first review I think the overall approach makes sense to me 👍 . The principles should be:

Only altering the final order if possible, but never the content. (I think this is the followed approach)
In case of failure in the ordering process, fallback to the original content (this is also covered, but I would prefer to have the new code wrapped in a try/except section that fallbacks to default in case of error)
Performance should not be considerable affected. I did some tests and this seems to be case, but I would prefer if you also do some tests: run some pages n times with/without the fix and compare the times.

Also, I left some comments

extruct/rdfa.py

tests/test_extruct.py

tests/test_uniform.py

ivanprado · 2020-06-04T10:13:17Z

Something also remaining:

Documenting clearly that this is a hack to fix the ordering and that it should be disabled once PyRDFA fixes itself.
Adding more tests with corner cases (e.g. bad namespace)

ivanprado · 2020-06-16T14:56:23Z

Hi @adityas114 .

Firstly, thank you again for collaborating with this fix. It is very appreciated 👍

I left some additional comments.

Copy of the original file

"test_rdfa_not_preserving_order" uses the "elysianfields_1.html" file now

Removing xfail tag from "test_uopengraph"

Add bad property to test for corner case

Optimized sort, fix for corner case (bad properties)

adityas114 · 2020-06-17T10:50:48Z

Hi @ivanprado .

I added some changes, please check them out. I believe I have incorporated all your suggestions - let me know if I missed something 😄

Also, I did a test to compare the performance. I called the extract method 100 times, with the 'elysianfields_1.html' file as parameter. Without patch, it ran in 13.070286s, and with the patch it ran in 13.093408s. Clearly, the performance is only marginally affected.

Let me know if you have more suggestions, I'd be happy to help 👍 😄

tests/test_extruct.py

extruct/rdfa.py

ivanprado

@adityas114 thank you again for these changes. I really appreciate. I think we are pretty close to having the PR ready to be merged, all my comments were addressed. I found a few new issues that you can find in the comments.

extruct/rdfa.py

tests/samples/songkick/elysianfields.html

tests/test_uniform.py

extruct/rdfa.py

adityas114 · 2020-07-07T13:08:11Z

I have made all the suggested changes. Please review 😄

ivanprado

@adityas114 It looks great 👏 ! Thank you very much for contributing 😄

I leave the issue open a while just in case somebody ones to review (/cc @Gallaecio @lopuhin ) and then I'll merge it.

lopuhin

Thanks for the PR @adityas114 , approach looks good overall 👍 Left some questions/suggestions below.

extruct/rdfa.py

lopuhin

Looks good and fixes an important problem, thanks @adityas114 👍

adityas114 added 4 commits May 26, 2020 16:51

Fix for issue scrapinghub#116

b2d6600

I fix the order in the json-ld string returned by rdflib by checking the correct order in the document object.

Fix for issue scrapinghub#116

f070e86

The test case 'test_rdfa_not_preserving_order' should not call extruct.extract for only RDFa, but for all formats, since the expected json string also has all formats.

Removed 'xfail' tag

6d58193

Removed 'xfail' tag from 'tagtest_rdfa_not_preserving_order' test case as issue scrapinghub#116 is fixed.

Fix for issue scrapinghub#116

248a2d9

I fix the order in the json-ld string returned by rdflib by checking the correct order in the document object.

adityas114 closed this May 26, 2020

adityas114 changed the title ~~Fix for issue #116~~ Fix for scrapinghub#116 May 26, 2020

adityas114 reopened this May 26, 2020

adityas114 mentioned this pull request May 26, 2020

RDFa ordering not preserved on duplicated properties #116

Closed

adityas114 added 6 commits May 28, 2020 01:53

Added xfail tag for 'test_uopengraph'

6ad394e

Opengraph output is not correct when I updated the 'elysianfields.html' file.

Added xfail tag for 'test_all'

ab3d34e

Opengraph output is not correct when I updated the 'elysianfields.html' file.

Update elysianfields.html

e4f5434

Updated to test issue scrapinghub#116

Update elysianfields.json

62b5d8b

Updated to test issue scrapinghub#116

Update rdfa.py

3780e02

Optimised performance of 'fixOrder' function by using xpath.

Update rdfa.py

23425a0

Fixed minor bugs

ivanprado reviewed Jun 3, 2020

View reviewed changes

extruct/rdfa.py Show resolved Hide resolved

ivanprado reviewed Jun 3, 2020

View reviewed changes

adityas114 added 7 commits June 17, 2020 14:54

Rename elysianfields.html to elysianfields_1.html

a05b1e9

Rename elysianfields.json to elysianfields_1.json

68f94de

Create elysianfields.html

d28ff32

Copy of the original file

Create elysianfields.json

32ca82d

Copy of the original file

Update test_extruct.py

e6c8572

"test_rdfa_not_preserving_order" uses the "elysianfields_1.html" file now

Update test_uniform.py

5887fca

Removing xfail tag from "test_uopengraph"

Update elysianfields_1.html

70508eb

Add bad property to test for corner case

Update rdfa.py

12798a4

Optimized sort, fix for corner case (bad properties)

ivanprado reviewed Jun 25, 2020

View reviewed changes

tests/test_extruct.py Outdated Show resolved Hide resolved

tests/test_extruct.py Outdated Show resolved Hide resolved

extruct/rdfa.py Outdated Show resolved Hide resolved

ivanprado reviewed Jun 25, 2020

View reviewed changes

extruct/rdfa.py Outdated Show resolved Hide resolved

extruct/rdfa.py Outdated Show resolved Hide resolved

tests/samples/songkick/elysianfields.html Outdated Show resolved Hide resolved

tests/test_uniform.py Outdated Show resolved Hide resolved

extruct/rdfa.py Show resolved Hide resolved

adityas114 and others added 5 commits July 6, 2020 21:34

Update test_extruct.py

0d177e5

Update rdfa.py

8193a1c

Update elysianfields_1.html

0f99c0f

Update test_uniform.py

5e8ce29

Restoring original elysianfields.html

66c7b89

ivanprado and others added 4 commits July 7, 2020 15:41

Cosmetic changes

4a4bfad

Merge branch 'master' into master

a91b73f

Fix a bug when head or html tags are not present

e02eace

Test case for checking isolation under crashes

fef751b

ivanprado approved these changes Jul 7, 2020

View reviewed changes

lopuhin reviewed Jul 8, 2020

View reviewed changes

extruct/rdfa.py Outdated Show resolved Hide resolved

extruct/rdfa.py Show resolved Hide resolved

extruct/rdfa.py Show resolved Hide resolved

Update rdfa.py

78c76a3

lopuhin approved these changes Jul 9, 2020

View reviewed changes

lopuhin merged commit bd49a5f into scrapinghub:master Jul 9, 2020

Fix for scrapinghub#116 #139

Fix for scrapinghub#116 #139

Uh oh!

Conversation

adityas114 commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ivanprado commented May 27, 2020

Uh oh!

lopuhin commented May 27, 2020

Uh oh!

adityas114 commented May 27, 2020

Uh oh!

adityas114 commented May 27, 2020

Uh oh!

Uh oh!

ivanprado left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivanprado commented Jun 4, 2020

Uh oh!

ivanprado commented Jun 16, 2020

Uh oh!

adityas114 commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivanprado left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adityas114 commented Jul 7, 2020

Uh oh!

ivanprado left a comment

Choose a reason for hiding this comment

Uh oh!

lopuhin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lopuhin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adityas114 commented May 26, 2020 •

edited

Loading

codecov bot commented May 26, 2020 •

edited

Loading

adityas114 commented Jun 17, 2020 •

edited

Loading