laszlocsontos / relinkr

reLinkR is the most advanced Java-based Open Source URL shortener
https://app.relinkr.io/
Apache License 2.0
4 stars 2 forks source link

Link table is empty despite having created links #80

Closed djavorszky closed 5 years ago

djavorszky commented 5 years ago

Steps to reproduce:

  1. Navigate to links page
  2. Create a new link with a valid URL
  3. Click on Save

Expected result: The link should show up in the table Actual result: The table remains empty (or no new entry gets added).

After trying it twice, I can see from the dashboard that the links have been created, as they do show up in the "Links" chart.

laszlocsontos commented 5 years ago

The link seems to have saved, but listing links then fails with NPE.

2019-09-02 07:26:04 default[20190901t135225]  "OPTIONS /v1/links?page=0&size=10&bust=1567409164903" 200
2019-09-02 07:26:05 default[20190901t135225]  2019-09-02 07:26:05.422 ERROR 1 --- [nio-8080-exec-5] io.relinkr.core.web.RestErrorHandler     : null
2019-09-02 07:26:05 default[20190901t135225]
2019-09-02 07:26:05 default[20190901t135225]  java.lang.NullPointerException: null
    at io.relinkr.link.web.LinkResource.of(LinkResource.java:114) ~[classes!/:1.0.0-SNAPSHOT]
    at io.relinkr.link.web.LinkResourceAssembler.instantiateResource(LinkResourceAssembler.java:68) ~[classes!/:1.0.0-SNAPSHOT]
    at io.relinkr.link.web.LinkResourceAssembler.instantiateResource(LinkResourceAssembler.java:38) ~[classes!/:1.0.0-SNAPSHOT]
    at org.springframework.hateoas.mvc.IdentifiableResourceAssemblerSupport.createResourceWithId(IdentifiableResourceAssemblerSupport.java:72) ~[spring-hateoas-0.25.1.RELEASE.jar!/:na]
    at org.springframework.hateoas.mvc.IdentifiableResourceAssemblerSupport.createResource(IdentifiableResourceAssemblerSupport.java:63) ~[spring-hateoas-0.25.1.RELEASE.jar!/:na]
    at org.springframework.hateoas.mvc.IdentifiableResourceAssemblerSupport.createResource(IdentifiableResourceAssemblerSupport.java:59) ~[spring-hateoas-0.25.1.RELEASE.jar!/:na]
    at io.relinkr.link.web.LinkResourceAssembler.toResource(LinkResourceAssembler.java:55) ~[classes!/:1.0.0-SNAPSHOT]
    at io.relinkr.link.web.LinkResourceAssembler.toResource(LinkResourceAssembler.java:38) ~[classes!/:1.0.0-SNAPSHOT]
    at org.springframework.data.web.PagedResourcesAssembler.createResource(PagedResourcesAssembler.java:208) ~[spring-data-commons-2.1.6.RELEASE.jar!/:2.1.6.RELEASE]
    at org.springframework.data.web.PagedResourcesAssembler.toResource(PagedResourcesAssembler.java:120) ~[spring-data-commons-2.1.6.RELEASE.jar!/:2.1.6.RELEASE]
    at io.relinkr.link.web.LinkResourceController.fetchLinks(LinkResourceController.java:163)

Questions to clarify:

djavorszky commented 5 years ago

So far I can confirm that links.getTargetUrl() returns null. The links journey starts when they are read from the database in LinkServiceImpl#fetchLinks. The returned Page<Link> already contains Links whose longUrl.targetUrl are null. As I can see, they are not updated until the NPE is thrown.

As longUrl is instantiated from a String from the database (which contains the correct value), something must go wrong during longUrl's creation.

I could only catch the creation of longUrl when the link is being added through the form. However, when it is read from the database, the object gets created somehow, with only its longUrl field filled out (and the UTM params, if they were set).

So right now my idea is that creating the links when read up from the database is not as complete as it should be.

djavorszky commented 5 years ago

As I can see, the issue here is that targetUrl is a computed value. When creating a variable from the link form, it gets computed, however, when Spring reads it from the database, it does not have information as to how it should be populated, so it leaves it blank.

I think there's two options:

  1. We also save the targetUrl to the database, though that wouldn't fix the links that were already created
  2. We post-process the links read from the database, though we'd need to do this for every method that does a read.

There's also the possibility that I'm missing something obvious :-)

What do you think @laszlocsontos, how should we handle this?

djavorszky commented 5 years ago

Wait, what if add a check to compute the property lazily? e.g. in link.getTargetUrl() we populate the field if null

laszlocsontos commented 5 years ago

Good job @djavorszky with the investigation!

I've actually screwed that up myself here.

The design principle I wished to follow was that objects should be immutable.

Well, at least quasi immutable, because JPA entities must have a no-arg constructor either way. This is because Hibernate sets fields directly through reflection and that's the reason why targetUrl eventually remains null.

I think the latter is more readable,as makes it obvious why we do that. Having lazy initialization in place seems to question the immutability of LongUrl. That was the very reason I boldly removed it and then seeing test cases passing I didn't think that I broke anything.

Please also add an extra test case to LinkRepositoryTest to ensure that out assumption that @javax.persistence.PostLoad will be fired when the entity is loaded. After all, we're testing the persistence here, not the logic in LongUrl itself.

djavorszky commented 5 years ago

Sounds good. I like the @PostLoad approach, will try to see if that works.

In the meantime I tried hacking together a test in LinkTest with reflection, and the solution by essentially adding the logic you mentioned - see them referenced above.

laszlocsontos commented 5 years ago

Many thanks Dani, I've just closed #83 and your commit https://github.com/laszlocsontos/relinkr/commit/29bcb97a6fdf9b6e949ab58e142f45523b0ec136 is the 500th in the line. I think that's kinda cool, isn't it. ;)

djavorszky commented 5 years ago

Thanks Laci! It was a learning experience for sure :)