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

SRAMP-29 StoredQuery support #494

Merged
merged 1 commit into from Sep 24, 2014

Conversation

Projects
None yet
2 participants
@brmeyer
Contributor

brmeyer commented Sep 24, 2014

No description provided.

+ @Override
+ protected void configureWorkspace() {
+ // TODO: What is the last arg?
+ AppCollection collection = addCollection("/s-ramp/query", "Stored Queries", MediaType.APPLICATION_ATOM_XML_FEED); //$NON-NLS-1$ //$NON-NLS-2$

This comment has been minimized.

@brmeyer

brmeyer Sep 24, 2014

Contributor

@EricWittmann, any idea what that "accepts" argument does? I can't find much info on it. Other than populating our /servicedocument, I'm not really understanding the Workspace need in general...

@brmeyer

brmeyer Sep 24, 2014

Contributor

@EricWittmann, any idea what that "accepts" argument does? I can't find much info on it. Other than populating our /servicedocument, I'm not really understanding the Workspace need in general...

This comment has been minimized.

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

See section 8.1 of http://www.rfc-base.org/txt/rfc-5023.txt for more info about the Workspace.

Section 8.3.4 specifically describes the 'accept' argument.

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

See section 8.1 of http://www.rfc-base.org/txt/rfc-5023.txt for more info about the Workspace.

Section 8.3.4 specifically describes the 'accept' argument.

This comment has been minimized.

@brmeyer

brmeyer Sep 24, 2014

Contributor

Awesome, thanks. So, I have it incorrect, since you post an entry...

@brmeyer

brmeyer Sep 24, 2014

Contributor

Awesome, thanks. So, I have it incorrect, since you post an entry...

+ * @throws SrampClientException
+ * @throws SrampAtomException
+ */
+ public QueryResultSet queryWithStoredQuery(StoredQuery storedQuery) throws SrampClientException, SrampAtomException {

This comment has been minimized.

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

Shouldn't fetching the results of a stored query simply require the name vs. the StoredQuery object? I think you'd want to hit the propery stored query API endpoint (as defined by the spec binding) instead of doing an ad-hoc query. The main reason is that the server may optimise the stored query results, so the client should leverage that.

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

Shouldn't fetching the results of a stored query simply require the name vs. the StoredQuery object? I think you'd want to hit the propery stored query API endpoint (as defined by the spec binding) instead of doing an ad-hoc query. The main reason is that the server may optimise the stored query results, so the client should leverage that.

This comment has been minimized.

@brmeyer

brmeyer Sep 24, 2014

Contributor

Very good point. I think I got too carried away with "reuse all the things."

@brmeyer

brmeyer Sep 24, 2014

Contributor

Very good point. I think I got too carried away with "reuse all the things."

+- sramp:queryName (string) mandatory
+- sramp:queryExpression (string) mandatory
+- sramp:propertyName (string) multiple
+

This comment has been minimized.

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

This is the first time the CND has changed since the first FSW version - this change may require an upgrade path.

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

This is the first time the CND has changed since the first FSW version - this change may require an upgrade path.

This comment has been minimized.

@brmeyer

brmeyer Sep 24, 2014

Contributor

So, that's a good question. Does that affect existing JCR instances, or does it simply make the node type available when first needed? I'll look into that -- not sure

@brmeyer

brmeyer Sep 24, 2014

Contributor

So, that's a good question. Does that affect existing JCR instances, or does it simply make the node type available when first needed? I'll look into that -- not sure

+ */
+public class JCRNodeToStoredQuery {
+
+ public void read(StoredQuery storedQuery, Node jcrNode) throws RepositoryException {

This comment has been minimized.

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

Some formatting issues with this method. Probably my fault - you copy/pasted some older (pre-formatting) code. :)

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

Some formatting issues with this method. Probably my fault - you copy/pasted some older (pre-formatting) code. :)

This comment has been minimized.

@brmeyer

brmeyer Sep 24, 2014

Contributor

I'm going to go ahead and lie, claiming that I pasted nothing ;)

@brmeyer

brmeyer Sep 24, 2014

Contributor

I'm going to go ahead and lie, claiming that I pasted nothing ;)

@@ -127,6 +127,10 @@ public void print(String formattedMessage, Object... params) {
public void setOutput(Writer output) {
this.writer = output;
}
+
+ protected Writer getOuptut() {

This comment has been minimized.

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

Typo alert.

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

Typo alert.

+ SrampAtomApiClient client = StoredQueryCommandUtil.client(this, getContext());
+ if (client == null) {
+ return false;
+ }

This comment has been minimized.

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

Bit of formatting trouble here.

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

Bit of formatting trouble here.

+ } catch (Exception e) {
+ print(Messages.i18n.format("DeleteStoredQueryCommand.Fail", name)); //$NON-NLS-1$
+ print("\t" + e.getMessage()); //$NON-NLS-1$
+ return false;

This comment has been minimized.

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

And here.

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

And here.

+ */
+ @Override
+ public int tabCompletion(String lastArgument, List<CharSequence> candidates) {
+ return StoredQueryCommandUtil.tabCompletion(this, getArguments(), getContext(), candidates);

This comment has been minimized.

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

Also here... ok fine just format all the new command classes! :)

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

Also here... ok fine just format all the new command classes! :)

+ }
+
+ try {
+ StoredQuery storedQuery = client.getStoredQuery(name);

This comment has been minimized.

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

I'll repeat the comment from earlier - I think we should actually hit the binding endpoint for the stored query results. I realize that might mean you can't easily re-use the s-ramp:query command here, but I think it's worth it.

@EricWittmann

EricWittmann Sep 24, 2014

Contributor

I'll repeat the comment from earlier - I think we should actually hit the binding endpoint for the stored query results. I realize that might mean you can't easily re-use the s-ramp:query command here, but I think it's worth it.

This comment has been minimized.

@brmeyer

brmeyer Sep 24, 2014

Contributor

+1

@brmeyer

brmeyer Sep 24, 2014

Contributor

+1

brmeyer added a commit that referenced this pull request Sep 24, 2014

Merge pull request #494 from brmeyer/SRAMP-29
SRAMP-29 StoredQuery support

@brmeyer brmeyer merged commit 361fc26 into ArtificerRepo:master Sep 24, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment