Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed cached schemas management in RefResolver. #263

Closed
wants to merge 26 commits into from
Closed

Fixed cached schemas management in RefResolver. #263

wants to merge 26 commits into from

Conversation

Alex-PK
Copy link

@Alex-PK Alex-PK commented May 5, 2016

Hi,

while trying out your package today I discovered that schema caching was not working very well, as the cache was not passed by reference.

The base draft-04 schema from json-schema.org was being downloaded multiple times (as it has a cyclic reference to itself), and the download hung on the second try, maybe due to a flaky connection on my side.

This patch "centralizes" the schemas cache in the RefResolver class into a private array, avoiding the need to pass it around in the functions calls. As they are all private, this shouldn't impact any other part of the system.

All the tests are passing.

Cheers.

@@ -30,6 +30,9 @@ class RefResolver
/** @var UriResolverInterface */
private $uriResolver;

/** @var JsonPointer[] */
private $cache = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use array() here for PHP 5.3 support

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I forgot about it. :)

@jojo1981
Copy link

jojo1981 commented May 6, 2016

Now the refResolver is not stateless anymore. The file caching will now be done in the RefResolver and will be persisted. So maybe add a public method to clear the cache?

@jojo1981
Copy link

jojo1981 commented May 6, 2016

By the way the UriRetriever is also caching the files? So why this needs to be done twice?

@Alex-PK
Copy link
Author

Alex-PK commented Jun 1, 2016

Sorry, I forgot a [] in the cache cleaning function (I am too much used to it... :) )

@bighappyface
Copy link
Collaborator

@Alex-PK thanks for your patience on this. Would you mind rebasing your work and squashing it down to a single commit?

@jojo1981 are you good with this?

@jojo1981
Copy link

jojo1981 commented Jun 3, 2016

@bighappyface I think the RefResolver should be stateless or a clearCache method should be implemented to clear the cache.

@Alex-PK
Copy link
Author

Alex-PK commented Jun 3, 2016

The clearCache method has been implemented in f5269bd. I am now studying how to rebase and squash without breaking anything, as I have never done it before. :)

@Alex-PK
Copy link
Author

Alex-PK commented Jun 4, 2016

Squashed and rebased. Sorry it took me so long.

@jojo1981
Copy link

jojo1981 commented Jun 5, 2016

@Alex-PK Because the RefResolver is not stateless anymore I think the documentation needs to be updated and explained the clearCache method should be used to clear the cache.

@Alex-PK
Copy link
Author

Alex-PK commented Jun 5, 2016

There is no documentation... :)
What should I update? The README.md?

@jojo1981
Copy link

jojo1981 commented Jun 6, 2016

@Alex-PK Yes.

@bighappyface
Copy link
Collaborator

@Alex-PK I am glad you got to learn about rebasing and squashing on this contribution.

I am good with the rebase but want to point out that your git user configuration is probably off.

The single commit in this PR is authored by you yet submitted as an identity different than your github account:

https://github.com/justinrainbow/json-schema/pull/263/commits (shows as two people in the message)

You can see the author info using the "patch" URL: https://patch-diff.githubusercontent.com/raw/justinrainbow/json-schema/pull/263.patch

@Alex-PK
Copy link
Author

Alex-PK commented Jun 12, 2016

@bighappyface Sorry, that's probably because I worked on the patch both on my work PC and my home one, and they have different configuration (email address) but with the same GitHub account.

I now updated the README.md but am unable to squash it again. When I use git rebase -i I get a list of all the changes in master, and if I squash the last commit I get a "could not apply" error.

@bighappyface
Copy link
Collaborator

@Alex-PK try the following:

# Checkout master
git checkout master
# Bring local master up to date
#  (assuming https://github.com/justinrainbow/json-schema is set to git remote "upstream")
git pull upstream master
# Checkout working branch
git checkout fix-resolver-cache
# Rebase working branch on master branch
git rebase upstream/master
# Go into interactive rebase to squash/fixup commits and isolate the last two commits
git rebase -i HEAD~2
# Use your system text editor to complete the rebase/squash and push to your fork

I am by no means a git expert but those steps should help you get to the right spot. Also, you should consider amending commits to keep one rolling commit in a branch while you work on it.

Tim B and others added 6 commits June 22, 2016 18:02
when inheriting from TypeConstraint to change the behaviour of `validateType` (e.g. to add support for custom types), this would switch from the extended class to the base class if type is an array of types
move MinMaxPropertiesTest to proper location
fix instancing wrong type, breaks inheritance
Fix: allow explicit 0 secfracs in datetime format
@Alex-PK
Copy link
Author

Alex-PK commented Jul 6, 2016

I think I made a mess worse than the un-rebased code. :/

@bighappyface
Copy link
Collaborator

Hey @Alex-PK I this PR may not be necessary anymore:

#290

Forgive me for not diving in deeper but are both PRs going for the same thing? If so, please close this PR and my apologies for taking a later PR over this one.

@Alex-PK Alex-PK closed this Oct 10, 2016
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.

7 participants