Add has_many association by simozhan · Pull Request #46 · product-definition-center/pdc-ruby-gem

simozhan · 2018-01-03T09:10:51Z

Implement simple has_many association which can help to cache result
and add some test cases.
As pdc gem only reads data from pdc server, this patch only implement
get related association behavior, not including post action.

Known issues:

Need more test cases.
Haven't change existing api which can use has_many association
like Release, will change it in future patches.

JIRA: PDC-1685

coveralls · 2018-01-03T09:13:50Z

Coverage decreased (-0.1%) to 98.79% when pulling 4e5a14d on simozhan:add_association_has_many into f06a9be on product-definition-center:master.

coveralls · 2018-01-04T01:03:11Z

Coverage decreased (-0.1%) to 98.79% when pulling e84d548 on simozhan:add_association_has_many into f06a9be on product-definition-center:master.

coveralls · 2018-01-04T01:38:59Z

Coverage decreased (-0.1%) to 98.79% when pulling 6d199ae on simozhan:add_association_has_many into f06a9be on product-definition-center:master.

coveralls · 2018-01-04T05:24:39Z

Coverage decreased (-0.1%) to 98.79% when pulling 13de43e on simozhan:add_association_has_many into f06a9be on product-definition-center:master.

coveralls · 2018-01-04T05:51:58Z

Coverage decreased (-0.1%) to 98.79% when pulling f06cb84 on simozhan:add_association_has_many into f06a9be on product-definition-center:master.

simozhan · 2018-01-04T07:33:41Z

'API rate limit exceeded' Errors. Probably after 1 hour, the retriggering build will be green.
And also @xiangge is fixing the 'API rate limit exceeded' in commit e7786d0
So the patch itself has no errors, adding reviewers anyway.

xiangge · 2018-01-14T15:14:13Z

+      elsif attribute?(name) then attribute(name)
      elsif predicate?(name)   then predicate(name)
      elsif setter?(name)      then set_attribute(name, args.first)
      else super


I met an error called "SystemStackError: stack level too deep" if write like this in belongs_to commit.
Maybe this association check should be after setter?(name) , I have changed this in that pr.

In my code, I haven't met this issue. Will try it with your codes later

I think the problem is you haven't include necessary codes in spec/support/fixtures.rb, like following:
` class CustomPrimaryKeyModel
@@ -37,6 +38,7 @@ class CustomPrimaryKeyModel
include PDC::Resource::Attributes
include PDC::Resource::Attributes
include PDC::Resource::Identity
include PDC::Resource::Identity
include PDC::Resource::Scopes
include PDC::Resource::Scopes

include PDC::Resource::Associations`

You need to include PDC::Resource::Associations` for the tested model

xiangge · 2018-01-14T15:42:23Z

+
+  class ProductVersion < Association
+    has_many :releases, uri: 'rest_api/v1/releases/?product_version=:product_version_id'
+  end


When I test with Release class, I add this below line:
has_many :release_variants, uri: 'v1/release-variants/?release=:release_id'
It works, and there is no "rest_api" before v1.

@xiangge Can u add more detail for this? You mean u add release_variants in a Release class and it doesn't need rest_api?

xiangge · 2018-01-14T15:44:36Z

+    class HasMany < Association
+      def initialize(*args)
+        super
+        @options.reverse_merge!(uri: "#{parent_path}/:#{foreign_key}/#{@name}/(:#{primary_key})")


Seems even we don't set an uri when init the association, this default uri can't work.
If so I prefer to remove this line

I think if u don't set the uri, this default one will be used. Maybe in the future we will consider to change the API in this way :)

Should this maybe be properly joined by a ruby helper function instead of using string interpolation? (Not sure what's out there). Use URI.join maybe?

OK, got it.
I think we need a comment here to let people know they should add "uri" when they init the association because this default setting can't work currently.

@romanofski The out for this uri is sth like product-versions/:product_version_id/releases/:release_id. It is the default uri, but actually in PDC server, we don't have this API, we need to customize the uri when defining has_many association. I haven't used URI, because this uri is just a string, it is part of the real URI, this URI includes the sites, and this uri string and params. URI implementation is mostly in lib/pdc/resource/identity.rb and lib/pdc/resource/path.rb

xiangge · 2018-01-14T15:58:00Z

+      end
+
+      def load
+        self


Maybe we don't need to set the "uri" in association, because in each classes it can parse the uri for us.
Here we just need to add the below line to instead of "self" I think.
klass.where(@options[:k_foreign_key] => @params[foreign_key])
And also when we create a association we need to add a var like k_foreign_key there.

Class Release has_many :release_variants, k_foreign_key: :release end

I think the uri is used for overriding. The default one is as the above one uri: "#{parent_path}/:#{foreign_key}/#{@name}/(:#{primary_key})" and u can customize it if your request uri is not following the normal one.

coveralls · 2018-01-16T05:50:56Z

Coverage decreased (-0.2%) to 98.761% when pulling 56de980 on simozhan:add_association_has_many into 7db33f4 on product-definition-center:master.

simozhan · 2018-01-16T05:53:59Z

Rebase the latest codes. And add two more test cases related to reflect and custom class name.

romanofski · 2018-01-17T03:47:26Z

@@ -0,0 +1,44 @@
+require 'active_support/dependencies'


Isn't this addition the same as the one in #47 ? Yet I can't see them being re-used. Any duplication going on or?

@romanofski Yes. That is duplication. @xiangge and I are all base on the master code and implement has_many and belongs_to respectively. We will rebase it after one patch is merged.

Ah I see. Fair enough.

romanofski · 2018-01-17T03:48:28Z

+        @options[:class_name].constantize if @options[:class_name]
+      end
+
+      # https://github.com/rails/rails/blob/70ac072976c8cc6f013f0df3777e54ccae3f4f8c/activerecord/lib/active_record/inheritance.rb#L132-L150


Would be useful to add a comment for the motivation here. Just the link points to a compute_type method which ... well not sure could be different?

Ok. I think it is imitating rails's implementation to compute the class. I copied this from https://github.com/balvig/spyke. Actually most of the codes are imitating spyke and do some tweaks for our use case. Ok. I will add some comments here :)

Hm... why are we not actually using spyke? Is there something we do different in this gem? I checked the README, but couldn't find an explanation there.

@romanofski spyke is a generic gem to Interact with REST services in an ActiveRecord-like manner. It requires activesupport 4.0, but pdc gem needs to run against activesupport 3.2.22. I guess it is because Errata use this version. Also pdc gem is specific for PDC server, we only need to support Get request at least for now, we don't need to support POST/PUT, etc. For example, this association implementation only supports get, no post action.

romanofski · 2018-01-17T03:50:00Z

+    class HasMany < Association
+      def initialize(*args)
+        super
+        @options.reverse_merge!(uri: "#{parent_path}/:#{foreign_key}/#{@name}/(:#{primary_key})")


Should this maybe be properly joined by a ruby helper function instead of using string interpolation? (Not sure what's out there). Use URI.join maybe?

romanofski · 2018-01-17T03:51:30Z


    def method_missing(name, *args, &block)
-      if attribute?(name) then attribute(name)
+      if association?(name) then association(name).load


Why are we calling load here?

I think this is to load the association itself, we can use this association to fetch data from PDC server and fill the data in this association later.

Wouldn't this cause problems if the load fails?

will load fail? I haven't thought out the case. The fetch action maybe fail or return empty data, but it can be handled correctly.

romanofski · 2018-01-17T03:53:55Z

+    VCR.eject_cassette
+  end
+
+  let(:site) { 'https://pdc.host.dev.eng.pek2.redhat.com' }


Yeah I still have a problem with the data as outlined in previous reviews. See #38 for my explanation.

Ok. I was thinking we have the conclusion that we don't be nervous with the internal info detail leak as it is not sensitive. But no problem for me to change it to example.com

Not sure who gave this conclusion. The URL is actually the least of my concern. My concern is how you avoid leaking internal data which should not get into the public?

hmm... I think the url is the most important one, without this people don't know where to get all these internal information. Wrt the sensitive data to be avoided leaking, maybe release_id/product/product_version are the sensitive data(I am not sure), i choose dummy product and a wrong named ceph product(which name is different with product server), so I am assuming it is not too bad for leaking internal data, does it make sense?

romanofski · 2018-01-17T03:55:58Z

+  end
+
+  describe 'association' do
+    it 'association independence' do


Don't really understand the purpose of this test. What are we testing here?

This is imitating from spyke :). I think the purpose is to test that we have has_many association releases, not others

romanofski · 2018-01-17T03:57:02Z

+  end
+
+  describe 'array like' do
+    it 'array like behavior' do


Maybe better:

it 'behaves like an array' do

Ack. Will change it

romanofski · 2018-01-17T03:57:59Z

+
+      product_version = Fixtures::ProductVersion.new(product_version_id: 'ceph-1')
+      releases = product_version.releases.where(release_id: 'ceph-1.3@rhel-7')
+      releases.any?


Should this be asserted here?

You mean assert at releases.any? or releases.to_a
Actually it is assert the endpoint is only requested once, because we have association

romanofski · 2018-01-17T05:30:06Z

+  end
+
+  class ProductVersion < Association
+    has_many :releases, uri: 'rest_api/v1/releases/?product_version=:product_version_id'


Isn't the URI given by the name spacing of the association? E.g.

pv = PDC::V1::Release::ProductVersion.find('foo')

which would translate the URL the above. So not really sure why an additional URI needs to be configured...

The URI is constructed by several parts, the configured site, the uri and the query parameter. The uri has default value, but it is not proper for current PDC server, we need to customize it. So that is why additional uri needed

coveralls · 2018-01-17T07:07:01Z

Coverage decreased (-0.2%) to 98.761% when pulling a42c614 on simozhan:add_association_has_many into 7db33f4 on product-definition-center:master.

xiangge

LGTM

simozhan · 2018-01-24T08:32:22Z

Dear reviewers, any new comments on this patch?

simozhan · 2018-01-24T14:50:47Z

@romanofski About the sensitive info issue, I am thinking maybe we can make the test cases internal and exclude test in gem repo, but let's think about it in another patch :)

romanofski · 2018-01-24T23:50:25Z

@simozhan I don't know... IMHO unless you have a plan it just seems that you push this decision into the future to make a plan. If it's acknowledged as a problem I suppose coming up with a mitigation process would take precedence over adding more features. No?

simozhan · 2018-01-25T08:54:13Z

@romanofski I created a new jira task in PDC-2541. Will deal with it as the next task. I will merge this patch now, that is really a long time, the rpm feature parity somehow waiting on this. We need to make a new gem to continue with other feature parity's task. Sorry about that and thanks for your review comments.

1. Implement simple has_many association which can help to cache result and add some test cases. As pdc gem only reads data from pdc server, this patch only implement get related association behavior, not including post action. 2. Change some rubocop configurations Known issues: 1. Need more test cases. 2. Haven't change existing api which can use has_many association like Release, will change it in future patches. JIRA: PDC-1685

simozhan force-pushed the add_association_has_many branch from 4e5a14d to e84d548 Compare January 4, 2018 00:59

simozhan force-pushed the add_association_has_many branch from e84d548 to 6d199ae Compare January 4, 2018 01:36

simozhan force-pushed the add_association_has_many branch from 6d199ae to 13de43e Compare January 4, 2018 05:20

simozhan force-pushed the add_association_has_many branch from 13de43e to f06cb84 Compare January 4, 2018 05:48

simozhan requested review from dacdownunder, romanofski, simonbaird, sthaha and xiangge January 4, 2018 07:34

xiangge reviewed Jan 14, 2018

View reviewed changes

simozhan force-pushed the add_association_has_many branch from f06cb84 to 56de980 Compare January 16, 2018 05:47

romanofski reviewed Jan 17, 2018

View reviewed changes

romanofski requested changes Jan 17, 2018

View reviewed changes

xiangge approved these changes Jan 18, 2018

View reviewed changes

simozhan force-pushed the add_association_has_many branch from 7d2b5b1 to a42c614 Compare January 25, 2018 08:58

simozhan merged commit 92a09e1 into product-definition-center:master Jan 25, 2018

Uh oh!

Conversation

simozhan commented Jan 3, 2018

Uh oh!

coveralls commented Jan 3, 2018

Uh oh!

coveralls commented Jan 4, 2018

Uh oh!

coveralls commented Jan 4, 2018

Uh oh!

coveralls commented Jan 4, 2018

Uh oh!

coveralls commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simozhan commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simozhan Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 16, 2018

Uh oh!

simozhan commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simozhan Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

coveralls commented Jan 4, 2018 •

edited

Loading

simozhan Jan 17, 2018 •

edited

Loading

simozhan Jan 17, 2018 •

edited

Loading

coveralls commented Jan 17, 2018 •

edited

Loading