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

Prevent generator api from being used in bulk request #293

Merged
merged 9 commits into from
May 25, 2016

Conversation

bserdar
Copy link
Contributor

@bserdar bserdar commented May 5, 2016

No description provided.

@coveralls
Copy link

coveralls commented May 5, 2016

Coverage Status

Coverage decreased (-0.002%) to 67.871% when pulling 6ba57d6 on bserdar:generator-not-bulk into fde46a4 on lightblue-platform:master.

@@ -17,6 +17,6 @@

LightblueBulkDataResponse bulkData(AbstractDataBulkRequest<AbstractLightblueDataRequest> requests) throws LightblueException;

<T> T data(AbstractLightblueDataRequest lightblueRequest, Class<T> type) throws LightblueException;
<T> T data(LightblueRequest lightblueRequest, Class<T> type) throws LightblueException;
Copy link
Member

@dcrissman dcrissman May 17, 2016

Choose a reason for hiding this comment

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

I feel strongly that the data api should be locked down to AbstractLightblueDataRequest implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't agree. Then there should be two interfaces, and two methods. Having
an interface, and then requiring the abstract implementation of it is ugly.

On Mon, May 16, 2016 at 7:00 PM, Dennis Crissman notifications@github.com
wrote:

In core/src/main/java/com/redhat/lightblue/client/LightblueClient.java
#293 (comment)
:

@@ -17,6 +17,6 @@

 LightblueBulkDataResponse bulkData(AbstractDataBulkRequest<AbstractLightblueDataRequest> requests) throws LightblueException;
  • T data(AbstractLightblueDataRequest lightblueRequest, Class type) throws LightblueException;
  • T data(LightblueRequest lightblueRequest, Class type) throws LightblueException;

I feel strongly that the data api should be locked down to
AbstractLightblueDataRequest implementations. The method is not designed to
work with other types of implementations.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/lightblue-platform/lightblue-client/pull/293/files/6ba57d62c328c422cf8891eca563e7ca7de8bac8#r63451488

Copy link
Member

Choose a reason for hiding this comment

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

AbstractLightblueDataRequest is the parent of all the data request objects implemented in the lightblue client. Specifically it knows how to build the rest uri.

@dcrissman
Copy link
Member

What if we created a marker interface that extended LightblueRequest for requests that can be bulk packaged? Say WithBulkSupport or something? Then the DataBulkRequest could only accept only requests with that interface?

@bserdar
Copy link
Contributor Author

bserdar commented May 17, 2016

+1

On Mon, May 16, 2016 at 7:06 PM, Dennis Crissman notifications@github.com
wrote:

What if we created a marker interface that extended LightblueRequest for
requests that can be bulk packaged? Say WithBulkSupport or something? Then
the DataBulkRequest could only accept only requests with that interface?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#293 (comment)

@coveralls
Copy link

coveralls commented May 19, 2016

Coverage Status

Coverage decreased (-0.004%) to 68.912% when pulling 0f30c10 on bserdar:generator-not-bulk into 74b7dc3 on lightblue-platform:master.

@coveralls
Copy link

coveralls commented May 20, 2016

Coverage Status

Coverage increased (+0.9%) to 67.542% when pulling a26850a on bserdar:generator-not-bulk into 90e35ae on lightblue-platform:master.

return null;
}
return body.toString();
public JsonNode getBodyJson() {
Copy link
Member

Choose a reason for hiding this comment

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

abstract?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

abstract, instead of return null? could be, but then I have to implement it
in all the concrete classes.

On Fri, May 20, 2016 at 12:19 PM, Dennis Crissman notifications@github.com
wrote:

In
core/src/main/java/com/redhat/lightblue/client/request/AbstractLightblueMetadataRequest.java
#293 (comment)
:

@@ -36,14 +54,7 @@ public String getRestURI(String baseServiceURI) {
}

 @Override
  • public String getBody() {
  •    JsonNode body = getBodyJson();
    
  •    if (body == null) {
    
  •        return null;
    
  •    }
    
  •    return body.toString();
    
  • public JsonNode getBodyJson() {

abstract?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/lightblue-platform/lightblue-client/pull/293/files/a26850a67fc0252d7f24a09ecf4c45fd80b40b2f#r64085150

@coveralls
Copy link

coveralls commented May 20, 2016

Coverage Status

Coverage increased (+0.9%) to 67.542% when pulling e7c6e5a on bserdar:generator-not-bulk into 90e35ae on lightblue-platform:master.

@dcrissman
Copy link
Member

Not merging per @bserdar's request, but +1 to this change.

@bserdar
Copy link
Contributor Author

bserdar commented May 20, 2016

@bvulaj , please take a look at the changes to bulk requests

@paterczm
Copy link
Contributor

I like the changes. Since there are compatibility concerns, please remember to bump the version, even if it's SNAPSHOT (not seeing version bump in the diff).

@bserdar
Copy link
Contributor Author

bserdar commented May 24, 2016

Version bumped.

On Tue, May 24, 2016 at 8:34 AM, Marek notifications@github.com wrote:

I like the changes. Since there are compatibility concerns, please
remember to bump the version, even if it's SNAPSHOT (not seeing version
bump in the diff).


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#293 (comment)

@coveralls
Copy link

Coverage Status

Coverage decreased (-66.7%) to 0.0% when pulling 7a717ae on bserdar:generator-not-bulk into 90e35ae on lightblue-platform:master.

@paterczm paterczm merged commit 408b62d into lightblue-platform:master May 25, 2016
@coveralls
Copy link

coveralls commented May 25, 2016

Coverage Status

Coverage increased (+0.9%) to 67.542% when pulling bd92ef0 on bserdar:generator-not-bulk into 90e35ae on lightblue-platform:master.

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.

None yet

4 participants