Skip to content

Method for clearing handler cache. - #187

Closed
gcirne wants to merge 2 commits into
strongloop:masterfrom
globocom:clear-handler-cache
Closed

Method for clearing handler cache.#187
gcirne wants to merge 2 commits into
strongloop:masterfrom
globocom:clear-handler-cache

Conversation

@gcirne

@gcirne gcirne commented Feb 17, 2014

Copy link
Copy Markdown
Contributor

This is needed when adding a model dynamically after the rest handler has been cached. In this situation, the model would not be reflected in the handler and would return 404.

@slnode

slnode commented Feb 17, 2014

Copy link
Copy Markdown

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@bajtos

bajtos commented Feb 17, 2014

Copy link
Copy Markdown
Member

@slnode ok to test

@slnode

slnode commented Feb 17, 2014

Copy link
Copy Markdown

Test FAILed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/937/

@gcirne

gcirne commented Feb 17, 2014

Copy link
Copy Markdown
Contributor Author

Hi @bajtos, apparently the jenkins build failed. But the Travis build is passing. Anything I can do to help fix it?

@bajtos

bajtos commented Feb 17, 2014

Copy link
Copy Markdown
Member

Could you please rebase your changes on top of the current master? It looks like your patch is based on loopback@1.6.1.

Hint:

$ git fetch upstream # upstream = strongloop/loopback
$ git rebase upstream/master
$ git push -f

@gcirne

gcirne commented Feb 17, 2014

Copy link
Copy Markdown
Contributor Author

I believe it is already based on current master.

$ git log
commit 1424e83c586e5f96f100f9cc4f6dd1fca09b62cd
Author: Guilherme Cirne <gcirne@gmail.com>
Date:   Mon Feb 17 14:27:35 2014 -0300

    Method for clearing handler cache.

commit e56574fd13265e2f725a7376d5ff71ebaea1bac0
Merge: 1dc4a3f 46b579d
Author: Raymond Feng <enjoyjava@gmail.com>
Date:   Fri Feb 14 11:36:17 2014 -0800

    Merge pull request #181 from strongloop/feature/user-token-rel

    Make sure User/AccessToken relations are set up by default

@gcirne

gcirne commented Feb 17, 2014

Copy link
Copy Markdown
Contributor Author
$ git remote -v
origin  git@github.com:globocom/loopback.git (fetch)
origin  git@github.com:globocom/loopback.git (push)
upstream    https://github.com/strongloop/loopback.git (fetch)
upstream    https://github.com/strongloop/loopback.git (push)
$ git fetch upstream
$ git checkout clear-handler-cache
$ git rebase upstream/master
Current branch clear-handler-cache is up to date.

@bajtos

bajtos commented Feb 17, 2014

Copy link
Copy Markdown
Member

Then it's a bug of our Jenkins setup :(

@rmg could you please investigate? Looks like this is the same issue we were discussing over the e-mail recently. Is there a way how to reset the Jenkins state to get this pull request built correctly?

@bajtos

bajtos commented Feb 17, 2014

Copy link
Copy Markdown
Member

Back to the patch itself. First of all, thank you @gcirne for submitting it!

AFAIK app.handler is intentionally kept undocumented, LB applications should not need to worry about the implementation details of handler caching.

If I recall correctly, we were discussing this problem with @ritch and/or @raymondfeng and the plan was to add an event when a new model is registered. The handler cache can listen on this event and invalidate REST handler when a new model is added.

@gcirne Could you please describe your use case? How does it happen that you are adding/modifying models after the first REST request was handled?

@gcirne

gcirne commented Feb 17, 2014

Copy link
Copy Markdown
Contributor Author

Sure. We basically want to add a new model dynamically by POSTing a JSON
Schema (http://json-schema.org/) which describes the new model.

We have a JsonSchema model with an afterSave hook registered. When we POST
a JSON Schema to /api/json-schemas the afterSave hook is executed and
registers the new model. At this point, we need to clear the REST handler
from the cache so that we can access the new model over the REST api.

The event solution you describe sounds good to me and fits perfectly with
our use case. Do you want me to modify this PR? I'm new to LB internals but
from what I've seen, this isn't a really big change.

Guilherme Machado Cirne
gcirne@gmail.com

On Mon, Feb 17, 2014 at 4:12 PM, Miroslav Bajtoš
notifications@github.comwrote:

Back to the patch itself. First of all, thank you @gcirnehttps://github.com/gcirnefor submitting it!

AFAIK app.handler is intentionally kept undocumented, LB applications
should not need to worry about the implementation details of handler
caching.

If I recall correctly, we were discussing this problem with @ritchhttps://github.com/ritchand/or
@raymondfeng https://github.com/raymondfeng and the plan was to add an
event when a new model is registered. The handler cache can listen on this
event and invalidate REST handler when a new model is added.

@gcirne https://github.com/gcirne Could you please describe your use
case? How does it happen that you are adding/modifying models after the
first REST request was handled?


Reply to this email directly or view it on GitHubhttps://github.com//pull/187#issuecomment-35313789
.

@rmg

rmg commented Feb 17, 2014

Copy link
Copy Markdown
Member

test please

@rmg

rmg commented Feb 17, 2014

Copy link
Copy Markdown
Member

Sorry, @gcirne, Jenkins seems to be quite confused at the moment. Easiest way to trigger a rebuild and have it actually build the right thing is to push another commit. :-(

@slnode

slnode commented Feb 17, 2014

Copy link
Copy Markdown

Test FAILed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/942/

@gcirne

gcirne commented Feb 17, 2014

Copy link
Copy Markdown
Contributor Author

Hmm, failed again (but Travis is fine). :(

@slnode

slnode commented Feb 17, 2014

Copy link
Copy Markdown

Test FAILed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/943/

@rmg

rmg commented Feb 17, 2014

Copy link
Copy Markdown
Member

Jenkins is still building the wrong thing.. :-/

Whoever commits the merge will just have to make sure they test locally when they do it, I guess.

@bajtos

bajtos commented Feb 18, 2014

Copy link
Copy Markdown
Member

The event solution you describe sounds good to me and fits perfectly with
our use case. Do you want me to modify this PR? I'm new to LB internals but
from what I've seen, this isn't a really big change.

That would be awesome!

@gcirne

gcirne commented Feb 18, 2014

Copy link
Copy Markdown
Contributor Author

@bajtos ok, I'll try to get it done today.

@ritch

ritch commented Feb 18, 2014

Copy link
Copy Markdown
Member

Sorry, but as @bajtos said this is exposing internals shouldn't be exposed. We might drastically refactor these in 2.0.

However clearing the handlers on an attached event sounds good. In that PR you could include this method if it is private.

@ritch ritch closed this Feb 18, 2014
@gcirne

gcirne commented Feb 18, 2014

Copy link
Copy Markdown
Contributor Author

Hey @ritch, I'm not really sure I agree that the current implementation is exposing internals. The clearHandlerCache function exists to abstract away the internals.

I was thinking of making RestAdapter listen to the attached event and then call app.clearHandlerCache('rest'). But if this function is "private", how would I go about that?

@gcirne gcirne mentioned this pull request Feb 18, 2014
@gcirne
gcirne deleted the clear-handler-cache branch February 19, 2014 13:59
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.

5 participants