Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/main/java/com/amihaiemil/docker/Containers.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import javax.json.JsonObject;
import java.io.IOException;
import java.util.Iterator;
import java.util.Map;

/**
* Containers API. This is also an Iterable over the running containers.
Expand Down Expand Up @@ -75,10 +76,25 @@ Container create(
Container create(final JsonObject container) throws IOException;

/**
* Return all the Containers, not only the running ones.
* Return all Containers, not only the running ones.
* @return Iterator over all the containers.
*/
Iterator<Container> all();

/**
* Whether to request the size of containers (fields SizeRw and SizeRootFs).
* @param withSize Return the size of containers (SizeRw and SizeRootFs).
* @return Containers with modified size flag.
*/
Containers withSize(final boolean withSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

@milux I think that this could be implemented as a decorator to Containers, which returns decorated Container objects with size information

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above, maybe another interface?

Copy link
Contributor

Choose a reason for hiding this comment

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

@milux If we have the fields SizeRw and SizeRootFs for more API elements, then I think that an WithSize or Sized (I can't think in a good name for this) interface would be fine. But also, I think that we could create these objects inside a Containers decorator which returns only Sized objects. @amihaiemil how about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And here is the decorator once again... see above. 😉


/**
* Filter these Containers.
* @param filters Filters to apply.
* @return Filtered containers.
* @see <a href="https://docs.docker.com/engine/api/v1.35/#operation/ContainerList">Docker API Docs</a>
*/
Containers filter(Map<String, Iterable<String>> filters);
Copy link
Contributor

Choose a reason for hiding this comment

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

@milux I think that this should be implemented as a decorator called Filtered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but I'm a bit puzzled because I did my implementation in line with the patterns I found in RtImages and ListedImages, and now you suggest a completely different pattern that has not been used in the library AFAICT.
Also, decorator patterns are applicable to one class only, because a decorator cannot expose functionality of multiple classes, right?
However, I think we could indeed improve code quality by defining this method in a separate interface (e.g. Filterable) and implementing that in all classes with that functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

@milux In this case we don't need essentially another interface; a Filtered would decorate a Containers object and execute the filtering of the values as needed. But we can develop a new Filterable interface, which will retain the filtering logic for all filterable elements (containers, images and so on). It is indeed a fine solution. @amihaiemil WDYT about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I think we have to get some things straight here, because afaik the decorator pattern is an approach that simply doesn't make any sense here. It's almost like saying "let's use the observer pattern here". Please let me explain why I think so:
We could use the decorator pattern, whereas our decorator contains more or less precisely the same code as ListedContainers, plus a bunch of proxy methods that forward calls to the original RtContainer instance. We cannot remove all() and iterator(), because they need information about the filters and/or size flag to construct the API request, and the underlying implementation is per definition unaware of the wrapping decorator when using this pattern. Thus, we end up with a lot of code duplication, because we need to implement all() and iterator() twice, once for RtContainer and once for the decorator.
Maybe I'm an idiot and got your excellent idea all wrong, but I believe there might be an implicit error in your imagination about how this should work. If you still think decorators are a good idea, then please elaborate a bit more on this.


/**
* Return the Docker engine where these Containers came from.
Expand Down
162 changes: 162 additions & 0 deletions src/main/java/com/amihaiemil/docker/ListedContainers.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
/**
* Copyright (c) 2018-2019, Mihai Emil Andronache
* All rights reserved.
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
* 1)Redistributions of source code must retain the above copyright notice,
* this list of conditions and the following disclaimer.
* 2)Redistributions in binary form must reproduce the above copyright notice,
* this list of conditions and the following disclaimer in the documentation
* and/or other materials provided with the distribution.
* 3)Neither the name of docker-java-api nor the names of its
* contributors may be used to endorse or promote products derived from
* this software without specific prior written permission.
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
* LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*/
package com.amihaiemil.docker;

import org.apache.http.client.HttpClient;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.utils.URIBuilder;

import java.net.URI;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;

/**
* Listed containers, which may have filters applied.
* @author Michael Lux ([email protected])
* @version $Id$
* @since 0.0.11
*/
final class ListedContainers extends RtContainers {
Copy link
Contributor

Choose a reason for hiding this comment

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

@milux Take a look to the comments above; after they are implemented this class may be not necessary and those behaviors can come back to RtContainers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, we could move this functionality to RtContainers, but this is against the good pattern that I found with similar APIs in that library, where Sub-API-calls go to an abstract base class whereas concrete implementations for filter etc. go to a ListedXYZ class.
Do you really consider this a good idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

@milux In this case we would have iterator and all methods moved to the abstract class. The other methods would be implemented by their corresponding interfaces

Copy link
Contributor Author

@milux milux Feb 20, 2020

Choose a reason for hiding this comment

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

You're aware that interfaces do only define a contract/interface (thus their name...) and cannot contain any implementation?
Interface-like constructs that contain actual implementations, best known as traits, or other concepts for multiple inheritance are not available in Java.
We could move the project to Kotlin, though, then there are traits. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And moving iterator and all to a base class that's not aware of filters and flags is not possible for the reasons explained above, unless you start to copy-paste the code to multiple levels of inheritance (or nesting in case of decorators) which is a terrible idea in terms of software design.

/**
* Container filters.
*/
private final Map<String, Iterable<String>> filters;
/**
* Whether to request the size of containers (fields SizeRw and SizeRootFs).
*/
private final boolean withSize;

/**
* Ctor.
* @param client The http client.
* @param uri The URI for this Containers API.
* @param dkr The docker entry point.
* @checkstyle ParameterNumber (2 lines)
*/
ListedContainers(final HttpClient client, final URI uri, final Docker dkr) {
this(client, uri, dkr, Collections.emptyMap(), false);
}

/**
* Ctor.
* @param client The http client.
* @param uri The URI for this Containers API.
* @param dkr The docker entry point.
* @param filters Container filter
* @param withSize Size query flag
* @checkstyle ParameterNumber (2 lines)
*/
ListedContainers(
final HttpClient client, final URI uri,
final Docker dkr, final Map<String, Iterable<String>> filters,
final boolean withSize
) {
super(client, uri, dkr);
this.filters = filters;
this.withSize = withSize;
}

@Override
public Iterator<Container> iterator() {
final URIBuilder uriBuilder = new UncheckedUriBuilder(
super.baseUri().toString().concat("/json")
);
if (this.withSize) {
uriBuilder.addParameter("size", "true");
}
final FilteredUriBuilder uri = new FilteredUriBuilder(
uriBuilder,
this.filters);

return new ResourcesIterator<>(
super.client(),
new HttpGet(uri.build()),
json -> new RtContainer(
json,
super.client(),
URI.create(
super.baseUri().toString() + "/" + json.getString("Id")
),
super.docker()
)
);
}

@Override
public Containers withSize(final boolean newWithSize) {
return new ListedContainers(
super.client(),
this.baseUri(),
this.docker(),
this.filters,
newWithSize
);
}

@Override
public Iterator<Container> all() {
final URIBuilder uriBuilder = new UncheckedUriBuilder(
super.baseUri().toString().concat("/json")
);
uriBuilder.addParameter("all", "true");
if (this.withSize) {
uriBuilder.addParameter("size", "true");
}
final FilteredUriBuilder uri = new FilteredUriBuilder(
uriBuilder,
this.filters);

return new ResourcesIterator<>(
super.client(),
new HttpGet(uri.build()),
json -> new RtContainer(
json,
super.client(),
URI.create(
super.baseUri().toString() + "/" + json.getString("Id")
),
super.docker()
)
);
}

@Override
public Containers filter(final Map<String, Iterable<String>> newFilters) {
final Map<String, Iterable<String>> merged = new HashMap<>(
this.filters
);
merged.putAll(newFilters);
return new ListedContainers(
super.client(),
this.baseUri(),
this.docker(),
merged,
this.withSize
);
}
}
53 changes: 18 additions & 35 deletions src/main/java/com/amihaiemil/docker/RtContainers.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,23 +27,21 @@

import org.apache.http.HttpStatus;
import org.apache.http.client.HttpClient;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.methods.HttpPost;
import org.apache.http.entity.StringEntity;
import org.apache.http.message.BasicHeader;
import javax.json.Json;
import javax.json.JsonObject;
import java.io.IOException;
import java.net.URI;
import java.util.Iterator;

/**
* Containers API.
* @author Mihai Andronache ([email protected])
* @version $Id$
* @since 0.0.1
*/
final class RtContainers implements Containers {
abstract class RtContainers implements Containers {

/**
* Apache HttpClient which sends the requests.
Expand Down Expand Up @@ -138,41 +136,26 @@ public Container create(
post.releaseConnection();
}
}

@Override
public Iterator<Container> all() {
return new ResourcesIterator<>(
this.client,
new HttpGet(this.baseUri.toString().concat("/json?all=true")),
json -> new RtContainer(
json,
this.client,
URI.create(
this.baseUri.toString() + "/" + json.getString("Id")
),
this.docker
)
);
}

@Override
public Iterator<Container> iterator() {
return new ResourcesIterator<>(
this.client,
new HttpGet(this.baseUri.toString().concat("/json")),
json -> new RtContainer(
json,
this.client,
URI.create(
this.baseUri.toString() + "/" + json.getString("Id")
),
this.docker
)
);
}

@Override
public Docker docker() {
return this.docker;
}


/**
* Get the (protected) HttpClient for subclasses.
* @return HttpClient.
*/
HttpClient client() {
return this.client;
}

/**
* Get the (protected) base URI for subclasses.
* @return URI.
*/
URI baseUri() {
return this.baseUri;
}
}
2 changes: 1 addition & 1 deletion src/main/java/com/amihaiemil/docker/RtDocker.java
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public Reader events() throws IOException, UnexpectedResponseException {

@Override
public final Containers containers() {
return new RtContainers(
return new ListedContainers(
this.client,
URI.create(this.baseUri.toString() + "/containers"),
this
Expand Down
Loading