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

Feature/add jcr resource reader #36

Merged
merged 7 commits into from
Nov 24, 2020

Conversation

royteeuwen
Copy link
Contributor

Initial implementation of a jcr resource reader in moco

@royteeuwen royteeuwen force-pushed the feature/add-jcr-resource-reader branch from 13b2450 to fbfde0d Compare August 1, 2020 09:09
Copy link
Collaborator

@pun-ky pun-ky left a comment

Choose a reason for hiding this comment

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

Generally looks good but I need to test it by my own. After that I will let you know of course. Thanks very much for PR. Greetings

@@ -102,7 +108,9 @@ private void stop() {
@Override
public void runScript(Resource resource) {
final StubScript script = new StubScript(resource, manager, this);

final JcrResourceReaderFactory jcrResourceReaderFactory = new JcrResourceReaderFactory(resolverAccessor);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is a purpose of MethodClosure here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is so that you can use the same syntax as Moco, just method names instead of object.methodName. The methodclosure exposes the object’s method as global function in the groovyscripts instead of resourceFactory.jcr

try {
result.set(IOUtils.toString(inputStream, StandardCharsets.UTF_8));
} catch (IOException e) {
LOG.warn("Could not read input stream", e);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe put path in message to make it contextual.
How about throwing exception here to make it more strict?

@royteeuwen
Copy link
Contributor Author

any update on this @pun-ky

@pun-ky
Copy link
Collaborator

pun-ky commented Aug 17, 2020

thanks for reminder ;)

today I will check it. hopefully in the evening.

Copy link
Collaborator

@marcinkp marcinkp left a comment

Choose a reason for hiding this comment

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

It was quite busy time for us. I found time for review.
Tested it locally, seems to be working. I would suggest to change is the MediaType returned text/plain as default.
Could you also please create PR for develop branch instead master?

Thanks


@Override
public MediaType getContentType(HttpRequest request) {
return MediaType.HTML_UTF_8;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if one would like to control returned mediatype?
I believe text/plain as default would be better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

owkey

@marcinkp marcinkp merged commit 337d75b into wttech:master Nov 24, 2020
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.

3 participants