Skip to content

links vs _links #690

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

Closed
gehreleth opened this issue Jan 20, 2018 · 10 comments
Closed

links vs _links #690

gehreleth opened this issue Jan 20, 2018 · 10 comments

Comments

@gehreleth
Copy link

gehreleth commented Jan 20, 2018

(spring 1.5.9)

I'm using both @RepositoryRestResource and regular @RestControllers returning instances of entities extending ResouceSupport. I've noticed that CRUD methods provided by repositories are exposing links as 'links' field while former ones expose them as '_links'. How to make then expose links in more consistent way?

UPD: If i return my entities via Collections.singletonList(), links field seem to have correct name.

@gregturn
Copy link
Contributor

Show me a controller.

@gehreleth
Copy link
Author

@RestController
@RequestMapping("/api/lazy-tree")
public class LazyTree {
  @Autowired
  private EntityLinks entityLinks;

  @Autowired
  private IStorageNodeRepository storageNodeRepository;

  @Bean
  public ResourceProcessor<Resource<StorageNode>> storageNodeProcessor() {
    return new EmbedLinksToStorageNode();
  }

  @GetMapping(value = "/children/{parentId}")
  @Transactional(readOnly = true)
  @Cacheable("lazy-tree")
  public Collection<NodeResource> populateChildren(@PathVariable Long parentId) {
    StorageNode parent = storageNodeRepository.findOne(parentId);
    return getValues(convertNodesToJson(parent.getChildren()));
  }

  @GetMapping(value = "/branch/{terminalNodeId}")
  @Transactional(readOnly = true)
  @Cacheable("lazy-tree")
  public Collection<NodeResource> populateBranch(@PathVariable Long terminalNodeId) {
    ArrayList<StorageNode> branch = new ArrayList<>();
    StorageNode node = storageNodeRepository.findOne(terminalNodeId);
    while (node != null) {
      branch.add(node);
      node = node.getParent();
    }
    Collections.reverse(branch); // Order : from root to leaf
    return Collections.singletonList(mergeEachBranchNodeWithSiblings(branch));
  }

  private NodeResource mergeEachBranchNodeWithSiblings(Collection<StorageNode> branch) {
    return mergeEachBranchNodeWithSiblings0(branch.iterator()).getRight();
  }

  private Pair<Long, NodeResource> mergeEachBranchNodeWithSiblings0(Iterator<StorageNode> branchItr) {
    Pair<Long, NodeResource> retVal = null;
    if (branchItr.hasNext()) {
      StorageNode sn = branchItr.next();
      Pair<Long, NodeResource> branchNode = convertNodeToJson(sn);
      final Pair<Long, NodeResource> nextBranchNode = mergeEachBranchNodeWithSiblings0(branchItr);
      if (nextBranchNode != null) {
        final Long nextBranchNodeId = nextBranchNode.getLeft();
        Collection<Pair<Long, NodeResource>> nextBranchNodeSiblings =
                convertNodesToJson(sn.getChildren()).stream().map(sibling ->
                        nextBranchNodeId.equals(sibling.getLeft())
                                ? nextBranchNode
                                : sibling)
                        .collect(Collectors.toCollection(ArrayList::new));
        branchNode = extendBranchNodeWithSiblings(sn, branchNode, nextBranchNodeSiblings);
      }
      retVal = branchNode;
    }
    return retVal;
  }

  private Pair<Long, NodeResource> extendBranchNodeWithSiblings(StorageNode sn,
                                                                Pair<Long, NodeResource> pair,
                                                                Collection<Pair<Long, NodeResource>> siblings)
  {
    NodeResource src = pair.getRight();
    BranchNodeResource retVal = new BranchNodeResource();
    retVal.setType(src.getType());
    retVal.setText(src.getText());
    retVal.setUploadTs(src.getUploadTs());
    retVal.setChildren(getValues(siblings));
    retVal.add(entityLinks.linkForSingleResource(StorageNode.class, sn.getId()).withSelfRel());
    return Pair.of(pair.getLeft(), retVal);
  }

  private Collection<Pair<Long, NodeResource>> convertNodesToJson(Collection<StorageNode> arg) {
    return arg.stream().map(this::convertNodeToJson).collect(Collectors.toCollection(ArrayList::new));
  }

  private Pair<Long, NodeResource> convertNodeToJson(StorageNode storageNode) {
    NodeResource retVal = new NodeResource();
    setCommonFields(retVal, storageNode);
    addStorageNodeLinks(storageNode, retVal);
    retVal.add(entityLinks.linkForSingleResource(StorageNode.class, storageNode.getId()).withSelfRel());
    return Pair.of(storageNode.getId(), retVal);
  }

  private void setCommonFields(NodeResource nodeRepr, StorageNode storageNode) {
    nodeRepr.setType(storageNode.getNodeType());
    nodeRepr.setText(storageNode.getText());
    nodeRepr.setUploadTs(storageNode.getUploadTs().toString());
  }

  private static void addStorageNodeLinks(StorageNode storageNode, ResourceSupport resource) {
    if (!isBranch(storageNode.getNodeType())) {
      if (storageNode.getNodeType() == NodeType.Image) {
        resource.add(linkTo(methodOn(Blobs.class).get(storageNode.getAquamarineId(),
                null, null, null, null, null)).withRel("as-image-fragment"));
      }
      resource.add(linkTo(methodOn(Blobs.class).getRaw(storageNode.getAquamarineId())).withRel("as-raw"));
    } else {
      resource.add(linkTo(methodOn(LazyTree.class).populateChildren(storageNode.getId())).withRel("lazy-children"));
    }
    resource.add(linkTo(methodOn(LazyTree.class).populateBranch(storageNode.getId())).withRel("lazy-branch"));
  }

  private static boolean isBranch(NodeType nodeType) {
    return nodeType == NodeType.Zip || nodeType == NodeType.Folder;
  }

  private static Collection<NodeResource> getValues(Collection<Pair<Long, NodeResource>> pairs) {
    return pairs.stream().map(Pair::getRight).collect(Collectors.toCollection(ArrayList::new));
  }

  private static class NodeResource extends ResourceSupport {
    private NodeType type;
    private String text;

    private String uploadTs;

    public NodeType getType() {
      return type;
    }

    public void setType(NodeType type) {
      this.type = type;
    }

    public String getText() {
      return text;
    }

    public void setText(String text) {
      this.text = text;
    }

    public String getUploadTs() {
      return uploadTs;
    }

    public void setUploadTs(String uploadTs) {
      this.uploadTs = uploadTs;
    }
  }

  private static class BranchNodeResource extends NodeResource {
    private Collection<NodeResource> children;

    public Collection<NodeResource> getChildren() {
      return children;
    }

    public void setChildren(Collection<NodeResource> children) {
      this.children = children;
    }
  }

  private static class EmbedLinksToStorageNode implements ResourceProcessor<Resource<StorageNode>> {
    @Override
    public Resource<StorageNode> process(Resource<StorageNode> resource) {
      addStorageNodeLinks(resource.getContent(), resource);
      return resource;
    }
  }
}

@gregturn
Copy link
Contributor

gregturn commented Jan 20, 2018

Some suggestions:

  1. Move bean definitions outside of this controller. Controllers are created on demand, and can be transient. Thus, aren't good candidates for bean definitions.
  2. Every endpoint that has a return type of Collection<Resource> should instead be using Resources<T>. When Jackson serializes the results, only things wrapped in ResourceSupport and its subtypes (Resource, Resources, and PagedResources) will form a proper HAL document. Any other return type will cause an improper rendering.
  3. Third, and not really related to your issue at hand--DON'T USE FIELD INJECTION. Use constructor injection instead. It's real simple and highly recommended by the Spring team. For more about this, please read http://olivergierke.de/2013/11/why-field-injection-is-evil/ by my manager.

Let me know if this helps with your results.

@gehreleth
Copy link
Author

gehreleth commented Jan 21, 2018

Wrapped these collections. (Also moved bean definitions to @Configuration classes).

  @GetMapping(value = "/children/{parentId}")
  @Transactional(readOnly = true)
  @Cacheable("lazy-tree")
  public Resources<NodeResource> populateChildren(@PathVariable Long parentId) {
    StorageNode parent = storageNodeRepository.findOne(parentId);
    return new Resources<>(getValues(convertNodesToJson(parent.getChildren())));
  }

  @GetMapping(value = "/branch/{terminalNodeId}")
  @Transactional(readOnly = true)
  @Cacheable("lazy-tree")
  public Resources<NodeResource> populateBranch(@PathVariable Long terminalNodeId) {

    ArrayList<StorageNode> branch = new ArrayList<>();
    StorageNode node = storageNodeRepository.findOne(terminalNodeId);
    while (node != null) {
      branch.add(node);
      node = node.getParent();
    }
    Collections.reverse(branch); // Order : from root to leaf
    return new Resources<>(Collections.singletonList(mergeEachBranchNodeWithSiblings(branch)));
  }

Now I get this stacktrace. I don't understand the cause at this moment. I'm not using any kind of XML serialization in this project.

2018-01-21 09:57:16.033  WARN 10216 --- [nio-8080-exec-5] .w.s.m.s.DefaultHandlerExceptionResolver : Failed to write HTTP message: org.springframework.http.converter.HttpMessageNotWritableException: Could not marshal [Resources { content: [links: [<http://localhost:8080/emerald/api/hal/storage-node/26>;rel="self"]], links: [] }]: null; nested exception is javax.xml.bind.MarshalException
 - with linked exception:
[com.sun.istack.internal.SAXException2: unable to marshal type "org.crownjewels.emerald.controller.LazyTree$BranchNodeResource" as an element because it is missing an @XmlRootElement annotation]
2018-01-21 09:57:16.048 ERROR 10216 --- [nio-8080-exec-5] o.s.boot.web.support.ErrorPageFilter     : Forwarding to error page from request [/api/lazy-tree/branch/40] due to exception [getOutputStream() has already been called for this response]

java.lang.IllegalStateException: getOutputStream() has already been called for this response
        at org.apache.catalina.connector.Response.getWriter(Response.java:621) ~[catalina.jar:9.0.0.M22]
        at org.apache.catalina.connector.ResponseFacade.getWriter(ResponseFacade.java:227) ~[catalina.jar:9.0.0.M22]
        at javax.servlet.ServletResponseWrapper.getWriter(ServletResponseWrapper.java:109) ~[servlet-api.jar:4.0.EDR-b01]
        at javax.servlet.ServletResponseWrapper.getWriter(ServletResponseWrapper.java:109) ~[servlet-api.jar:4.0.EDR-b01]
        at org.springframework.security.web.util.OnCommittedResponseWrapper.getWriter(OnCommittedResponseWrapper.java:149) ~[spring-security-web-4.2.3.RELEASE.jar:4.2.3.RELEASE]
        at org.springframework.boot.autoconfigure.web.ErrorMvcAutoConfiguration$SpelView.render(ErrorMvcAutoConfiguration.java:227) ~[spring-boot-autoconfigure-1.5.9.RELEASE.jar:1.5.9.RELEASE]
        at org.springframework.web.servlet.DispatcherServlet.render(DispatcherServlet.java:1286) ~[spring-webmvc-4.3.13.RELEASE.jar:4.3.13.RELEASE]
        at org.springframework.web.servlet.DispatcherServlet.processDispatchResult(DispatcherServlet.java:1041) ~[spring-webmvc-4.3.13.RELEASE.jar:4.3.13.RELEASE]
        at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:984) ~[spring-webmvc-4.3.13.RELEASE.jar:4.3.13.RELEASE]
        at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:901) ~[spring-webmvc-4.3.13.RELEASE.jar:4.3.13.RELEASE]
        at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:970) ~[spring-webmvc-4.3.13.RELEASE.jar:4.3.13.RELEASE]
        at org.springframework.web.servlet.FrameworkServlet.doGet(FrameworkServlet.java:861) ~[spring-webmvc-4.3.13.RELEASE.jar:4.3.13.RELEASE]

.... Irrelevant ...

@gehreleth
Copy link
Author

gehreleth commented Jan 21, 2018

UPD: If I add produces = {MediaType.APPLICATION_JSON_VALUE, "application/hal+json"} qualifier to these mappings, this works, but nested links are marshalled as "_links". That's exactly what I've tried to avoid initially, because @RepositoryRestResource marshal them as "links" and I want to keep my frontend API more consistent.

@gregturn
Copy link
Contributor

In a browser it will try XML before JSON. curl will go straight to JSON. This is classic Spring MVC. You can either use produces as you have done. Or simply append ".json" to the route.

Regarding the output I'm confused. _links is HAL. links is not HAL, but instead a non standard representation.

@gehreleth
Copy link
Author

gehreleth commented Jan 21, 2018

I've already figured out how to get rid of this exception (I've set produces attribute in @RestController). However, I'm confused about what you said about HAL. I've got a @RepositoryRestResource

@RepositoryRestResource(path = "storage-node", excerptProjection = IBrief.class)
public interface IStorageNodeRepository extends JpaRepository<StorageNode, Long> {
  @Query("FROM StorageNode sn WHERE sn.parent is null order by sn.uploadTs ASC")
  List<StorageNode> findAllRootNodes();
}

This is sample json+hal generated by it. It has "links" instead of "_links". I assumed that spring library will generate 100% pedantic json+hal and tried to mimic it.

{
  "links" : [ {
    "rel" : "first",
    "href" : "http://localhost:8080/emerald/api/hal/storage-node?page=0&size=20"
  }, {
    "rel" : "self",
    "href" : "http://localhost:8080/emerald/api/hal/storage-node{?page,size,sort,projection}"
  }, {
    "rel" : "next",
    "href" : "http://localhost:8080/emerald/api/hal/storage-node?page=1&size=20"
  }, {
    "rel" : "last",
    "href" : "http://localhost:8080/emerald/api/hal/storage-node?page=19&size=20"
  }, {
    "rel" : "profile",
    "href" : "http://localhost:8080/emerald/api/hal/profile/storage-node"
  }, {
    "rel" : "search",
    "href" : "http://localhost:8080/emerald/api/hal/storage-node/search"
  } ],
  "content" : [ {
    "text" : "src.zip",
    "nodeType" : "Zip",
    "links" : [ {
      "rel" : "self",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/26"
    }, {
      "rel" : "storageNode",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/26{?projection}"
    }, {
      "rel" : "parent",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/26/parent"
    }, {
      "rel" : "imageMetadata",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/26/imageMetadata"
    }, {
      "rel" : "children",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/26/children"
    } ]
  }, {
    "text" : "src",
    "nodeType" : "Folder",
    "links" : [ {
      "rel" : "self",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/27"
    }, {
      "rel" : "storageNode",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/27{?projection}"
    }, {
      "rel" : "parent",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/27/parent"
    }, {
      "rel" : "imageMetadata",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/27/imageMetadata"
    }, {
      "rel" : "children",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/27/children"
    } ]
  }, {
    "text" : "Documents",
    "nodeType" : "Folder",
    "links" : [ {
      "rel" : "self",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/28"
    }, {
      "rel" : "storageNode",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/28{?projection}"
    }, {
      "rel" : "parent",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/28/parent"
    }, {
      "rel" : "imageMetadata",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/28/imageMetadata"
    }, {
      "rel" : "children",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/28/children"
    } ]
  }, {
    "text" : "1.jpg",
    "nodeType" : "Image",
    "links" : [ {
      "rel" : "self",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/29"
    }, {
      "rel" : "storageNode",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/29{?projection}"
    }, {
      "rel" : "parent",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/29/parent"
    }, {
      "rel" : "imageMetadata",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/29/imageMetadata"
    }, {
      "rel" : "children",
      "href" : "http://localhost:8080/emerald/api/hal/storage-node/29/children"
    } ]
  }, 
/* ........ Irrelevant ............*/
 } ],
  "page" : {
    "size" : 20,
    "totalElements" : 381,
    "totalPages" : 20,
    "number" : 0
  }
}


@gregturn
Copy link
Contributor

You are referring to a Spring Data repository. If this is Spring Data REST, you'll find that all the REST controllers leveraging scanned repositories return ResourceSupport-compatible types. When you don't, you get non-HAL stuff.

Please read the spec if you don't believe me when I say that fragment up above isn't HAL.

http://stateless.co/hal_specification.html

@gehreleth
Copy link
Author

How to make Spring Data repository return valid HAL then? Did I overlook something in spring data repositories' initialization?

@gehreleth
Copy link
Author

After giving it some thought, I've discovered this fragment.

@Bean
  public RepositoryRestConfigurer repositoryRestConfigurer() {
    return new RepositoryRestConfigurerAdapter() {
      @Override
      public void configureRepositoryRestConfiguration(RepositoryRestConfiguration config) {
        config.setBasePath("/api/hal");
        config.setDefaultMediaType(MediaType.APPLICATION_JSON_UTF8);
      }
    };
  }

Removing default media type did the job. I don't remember exactly under which circumstances I've added this. Maybe I had to explicitly say ;charset=utf8 in each response to deal with russian strings. You can close this issue.

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

No branches or pull requests

2 participants