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 mapping to avoid infinite loop #15

Merged
merged 4 commits into from Sep 8, 2015
Merged

Conversation

rkusuma
Copy link
Contributor

@rkusuma rkusuma commented Sep 3, 2015

Hi @guylabs,

Based on what you said in #12
I just creating your solution. Please kindly check if it right or not.

Romy Kusuma added 2 commits September 3, 2015 16:52
1. Add map object with self link as key and array of linkName as value
2. Skip fetching data if map[self] contain linkName
@@ -67,6 +67,8 @@ angular.module("spring-data-rest").provider("SpringDataRestAdapter", function ()
},

$get: ["$injector", function ($injector) {
// map contain self link as key and a list of linkNames which were already fetched
Copy link
Owner

Choose a reason for hiding this comment

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

I would write something like:

/**
* Link map which contains the 'self' link as key and the list of already fetched link names as value. This is used to 
* check that every link on each entity is only fetched once to avoid an infinite recursive loop.
*/
var linkMap = {}

Then it is more clear for that the map is used. And I would also rename the map variable to linkMap.

@guylabs
Copy link
Owner

guylabs commented Sep 5, 2015

You should also reinitialize the map at this line: https://github.com/guylabs/angular-spring-data-rest/pull/15/files#diff-f3122e396df944c9b7692dbac86ded5bR328 as if not, then all entity links are only fetched once as the map is not cleared for each processing.

And could you also add some tests to the test suite which tests the following use cases:

  • Have a structure the same as you have which gives an infinite loop and then test that the entities are properly fetched.
  • Process the same response twice but the backend sends not the same response for one specific link which will be automatically fetched. This is to check that map is reinitialized properly.

Please have a look at the test directory as there you will find all the needed tests.

Thanks a lot for the pull request!

Regards,

Guy

@rkusuma
Copy link
Contributor Author

rkusuma commented Sep 7, 2015

Hi @guylabs
I already initialized the map in this line https://github.com/rkusuma/angular-spring-data-rest/blob/master/src/angular-spring-data-rest-provider.js#L363

Could you tell me what you mean

You should also reinitialize the map at this line: https://github.com/guylabs/angular-spring-data-rest/pull/15/files#diff-f3122e396df944c9b7692dbac86ded5bR328 as if not, then all entity links are only fetched once as the map is not cleared for each processing.

Thanks

1. Update naming convention
2. Use config attribute to get self href
@guylabs
Copy link
Owner

guylabs commented Sep 7, 2015

Hi Romy,

you mean line https://github.com/rkusuma/angular-spring-data-rest/blob/master/src/angular-spring-data-rest-provider.js#L63 right? Well the thing is that the linkMap is just instantiated once in the lifecycle and if you use the SpringDataRestAdapter multiple times and process multiple resources the linkMap never gets cleared or reinitialized. That is why I said this in the comment. This ensures that we are able to fetch the links twice. (This should also be tested in a test such that we see that this works).

Do you know what I mean?

Thanks and regards,

Guy

@rkusuma
Copy link
Contributor Author

rkusuma commented Sep 8, 2015

Hi @guylabs
I know what you mean.
I already make sure linkMap is empty object when process function called from SpringDataRestAdapter

You can see in this line https://github.com/rkusuma/angular-spring-data-rest/blob/master/src/angular-spring-data-rest-provider.js#L370

Is that what you mean?
In your comment you not specified the line code, are you give the wrong link ?

You should also reinitialize the map at this line: https://github.com/guylabs/angular-spring-data-rest/pull/15/files#diff-f3122e396df944c9b7692dbac86ded5bR328 as if not, then all entity links are only fetched once as the map is not cleared for each processing.

Thanks

1. Create mock data
2. Add test cases to test infinite loop
@rkusuma
Copy link
Contributor Author

rkusuma commented Sep 8, 2015

@guylabs
Please kindly review my unit test.

@guylabs
Copy link
Owner

guylabs commented Sep 8, 2015

Hi Romy,

sorry for not seeing the initialization of the map. So there everything is ok :). And the tests are also good. Nice work and thanks a lot for you contribution.

I will now merge it to the master and then you are able to use it right away in your project.

Thanks and regards,

Guy

guylabs added a commit that referenced this pull request Sep 8, 2015
Add mapping to avoid infinite loop
@guylabs guylabs merged commit 7438358 into guylabs:master Sep 8, 2015
@rkusuma
Copy link
Contributor Author

rkusuma commented Sep 8, 2015

Thanks Guy..
Should u release it to 0.4.3 ?

@guylabs
Copy link
Owner

guylabs commented Sep 8, 2015

I can do a 0.4.3 release this evening if you want to. In the mean time you can have a look at this post and see how to use the master in bower: http://guylabs.ch/2015/05/08/get-latest-master-changes-of-a-bower-dependency/

@rkusuma
Copy link
Contributor Author

rkusuma commented Sep 8, 2015

Okay.. I'll wait 0.4.3 release and use master for temporary.
Thanks Guy

@guylabs
Copy link
Owner

guylabs commented Sep 8, 2015

Hi Romy,

I just released the 0.4.3 version. Hope now everything works as you need it :).

And thanks again for your contribution!

Regards,

Guy

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

2 participants