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

Add the vitruv client server implementation from Thomas Mayer #584

Closed
wants to merge 19 commits into from

Conversation

TomWerm
Copy link
Member

@TomWerm TomWerm commented Mar 29, 2023

No description provided.

@TomWerm TomWerm requested a review from a team as a code owner March 29, 2023 15:28
Copy link
Member

@tsaglam tsaglam left a comment

Choose a reason for hiding this comment

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

Is there a reason that we are adding spark-core-2.9.4.jar as a jar and not as a (maven) dependency?

@tsaglam tsaglam requested a review from a team March 30, 2023 16:41
@TomWerm
Copy link
Member Author

TomWerm commented Mar 31, 2023

Is there a reason that we are adding spark-core-2.9.4.jar as a jar and not as a (maven) dependency?

The maven dependency does not work out of the box, I am currently in contact with the author about that.

Copy link
Contributor

@h4uges h4uges left a comment

Choose a reason for hiding this comment

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

reviewed common code

checkState(!isClosed(), "View is already closed");
}

void checkState(boolean state, String msg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably have libraries for this. If not functions like this should be placed in util.

}
}

void checkArgument(boolean arg, String msg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably have libraries for this. If not functions like this should be placed in util.

}
}

private void addChangeListeners(Notifier notifier) {
Copy link
Contributor

Choose a reason for hiding this comment

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

static

Comment on lines 211 to 217
if (notifier instanceof ResourceSet n) {
n.getResources().forEach(this::addChangeListeners);
} else if (notifier instanceof Resource n) {
n.getContents().forEach(this::addChangeListeners);
} else if (notifier instanceof EObject n) {
n.eContents().forEach(this::addChangeListeners);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

use better variable names than n

.toString().replace("\\", "/");
}

private static String prepareId(String id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the method-name fitting and specific enough?

public class ResourceDeserializer extends JsonDeserializer<Resource> {

@Override
public Resource deserialize(JsonParser p, DeserializationContext c) throws IOException, JacksonException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use describing variable names.

/**
* Contains utility functions to work with {@link Resource}s.
*/
public class ResourceUtils {
Copy link
Contributor

Choose a reason for hiding this comment

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

Split ResourceUtils and Utils for (de)serializing a resource

public class ResourceSetDeserializer extends JsonDeserializer<ResourceSet> {

@Override
public ResourceSet deserialize(JsonParser p, DeserializationContext c) throws IOException, JacksonException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use describing variable names.

@Override
public VitruviusChange deserialize(JsonParser p, DeserializationContext c) throws IOException, JacksonException {
var rootNode = p.getCodec().readTree(p);
var type = c.readTreeAsValue((TextNode) rootNode.get("changeType"), ChangeType.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Make these constants static final and define them in one location. To make sure serializing and deserializing code uses the same names.
changeType, vChanges, content, uri, ...


VitruviusChange change = null;
if (type == ChangeType.TRANSACTIONAL) {
var json = ((TextNode)rootNode.get("eChanges")).asText();
Copy link
Contributor

Choose a reason for hiding this comment

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

json unspecific variable name

Copy link
Contributor

@h4uges h4uges left a comment

Choose a reason for hiding this comment

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

Reviewed server code.

I would rework the design of the REST-API. Make full use of the methods GET, POST, DELETE, PUT and PATCH instead of adding a new path for each intended operation. Also i would suggest putting the UUID in the path of the request instead of the header.

InternalVirtualModel model;

public ChangePropagationEndpoint(InternalVirtualModel model) {
super("/vsum/change", ContentTypes.APPLICATION_JSON);
Copy link
Contributor

Choose a reason for hiding this comment

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

store /vsum/change in a constant


@Override
public Void handleRequest(Request request, Response response) {
var change = JsonMapper.deserialize(request.body(), VitruviusChange.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should handle errors during de-serialization.

public class CloseViewEndpoint extends PostEndpoint {

public CloseViewEndpoint() {
super("/vsum/view/closed");
Copy link
Contributor

Choose a reason for hiding this comment

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

store path in a constant

/**
* This endpoint closes a {@link tools.vitruv.framework.views.View View}.
*/
public class CloseViewEndpoint extends PostEndpoint {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest providing this functionality as a DELETE request to /vsum/view

public class HealthEndpoint extends GetEndpoint {

public HealthEndpoint() {
super("/health");
Copy link
Contributor

Choose a reason for hiding this comment

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

use constant

public class UpdateViewEndpoint extends GetEndpoint {

public UpdateViewEndpoint() {
super("/vsum/view/update");
Copy link
Contributor

Choose a reason for hiding this comment

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

make constant

private final InternalVirtualModel model;

public ViewEndpoint(InternalVirtualModel model) {
super("/vsum/view");
Copy link
Contributor

Choose a reason for hiding this comment

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

make constant

Comment on lines 37 to 42
if (types.stream().noneMatch(it -> it.getName().equals(viewTypeName))) {
notFound("View Type with name " + viewTypeName + " not found!");
}

//Get selector and select every element
var selector = model.createSelector(types.stream().filter(e -> e.getName().equals(viewTypeName)).findFirst().get());
Copy link
Contributor

Choose a reason for hiding this comment

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

query for viewType is kind of duplicated

private final InternalVirtualModel model;

public ViewTypesEndpoint(InternalVirtualModel model) {
super("/vsum/viewtypes");
Copy link
Contributor

Choose a reason for hiding this comment

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

make constant


private static final HashMap<String, View> cache = new HashMap<>();

public static void addView(String uuid, View view) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public static void addView(String uuid, View view) {
public static void addView(String uuid, View view) {

Copy link
Contributor

@h4uges h4uges left a comment

Choose a reason for hiding this comment

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

reviewed client code:

  • change dependency of spark to a maven dependency
  • put some more effort in the OO-structure of the Views, ChangeDerivingRemoteView and ChangeRecordeingRemoteView probably can be assembled from existing code if RemoteView implements the right interfaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

Change to maven dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

should probably be ignored?

* A {@link VitruvRemoteConnection} acts as a {@link HttpClient} to forward requests to a vitruvius server, where a vitruvius instance is hosted.
* This enables the ability to perform actions on this remote vitruvius instance.
*/
public class VitruvRemoteConnection {
Copy link
Contributor

Choose a reason for hiding this comment

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

extract an interface

*/
public List<String> getViewTypes() throws BadServerResponseException {
var request = HttpRequest.newBuilder()
.uri(URI.create(String.format("http://%s:%d/%s", url, port, VIEW_TYPES))).GET().build();
Copy link
Contributor

Choose a reason for hiding this comment

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

extract method for URI creation

}

var rSet = JsonMapper.deserialize(response.body(), ResourceSet.class);
return new RemoteView(response.headers().firstValue(Headers.VIEW_UUID).get(), rSet, this);
Copy link
Contributor

Choose a reason for hiding this comment

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

claimOne instead of firstValue

Comment on lines 127 to 135
private VitruviusChange findChanges(Resource oldState, Resource newState) {
if (oldState == null) {
return resolutionStrategy.getChangeSequenceForCreated(newState);
} else if (newState == null) {
return resolutionStrategy.getChangeSequenceForDeleted(oldState);
} else {
return resolutionStrategy.getChangeSequenceBetween(newState, oldState);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

check code format

Comment on lines 28 to 32
protected final String uuid;
protected final VitruvRemoteConnection remoteConnection;

protected ResourceSet viewSource;
protected boolean modified = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hide these fields if possible.

allChanges.add(changes);
}
});
base.remoteConnection.propagateChanges(base.uuid, VitruviusChangeFactory.getInstance().createCompositeChange(allChanges));
Copy link
Contributor

Choose a reason for hiding this comment

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

don't access internals of a view

*/
public class ChangeDerivingRemoteView implements CommittableView {

private final RemoteView base;
Copy link
Contributor

Choose a reason for hiding this comment

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

use interface instead of concrete class

*/
public class ChangeRecordingRemoteView implements CommittableView {

private final RemoteView base;
Copy link
Contributor

Choose a reason for hiding this comment

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

use interface instead of direct class

@h4uges
Copy link
Contributor

h4uges commented Apr 26, 2023

Final comment of my review: This PR should be supplemented with tests (unit + integration).

Vitruv Remote: Client-Server Implementation
@TomWerm
Copy link
Member Author

TomWerm commented Aug 28, 2023

Outdated with #599

@TomWerm TomWerm closed this Aug 28, 2023
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.

6 participants