Avoid queries for ServiceProvider with blank issuer

[mitchellhenke]

🛠 Summary of changes

Small performance improvement to avoid queries for ServiceProvider where issuer is not present. We see this sometimes around SP requests based on looking into NewRelic metrics and local testing to confirm. Also moves a frequently used Regexp into a constant to avoid having to re-create it every time.

[aduth]

Technically we could make this a single if without the else and have this assign as nil with working memoization, though I think maybe this a good instance of being overly explicit due to how confusing the conditional assignment can be in Ruby.

    @saml_request_service_provider =
      if current_issuer.present?
        ServiceProvider.find_by(issuer: current_issuer)
      end

[mitchellhenke]

Ah, you're right, that hadn't occurred to me. I kind of prefer the explicitness though.

[Sgtpluck]

i have a slight preference for the explicitness as well!

[Sgtpluck]

we could add a test that checks whether the find_by method is called, but as the value of the assignment doesn't change, that doesn't feel blocking to me

[mitchellhenke]

we could add a test that checks whether the find_by method is called, but as the value of the assignment doesn't change, that doesn't feel blocking to me

I had assumed we didn't have many tests around not querying, but we do have this one. I've added a test for the OIDC case, but the SAML concern doesn't have its own test file currently, so I opted against it.

[zachmargolis]

we could add a test that checks whether the find_by method is called, but as the value of the assignment doesn't change, that doesn't feel blocking to me

I had assumed we didn't have many tests around not querying, but we do have this one. I've added a test for the OIDC case, but the SAML concern doesn't have its own test file currently, so I opted against it.

We also have a QueryTracker class that can track queries by table (so say, if we switched methods to use like .where instead of .find_by it would still detect)

identity-idp/spec/features/openid_connect/openid_connect_spec.rb

Lines 1089 to 1095 in 919b856

	it 'does not make too many queries' do
	queries = QueryTracker.track do
	visit_idp_from_ial1_oidc_sp
	end
	expect(queries[:service_providers].count).to be <= 6
	end