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

Additional tests for getting the interceptor bindings from the InvocationContext #522

Merged
merged 2 commits into from
Jan 11, 2024

Conversation

Azquelt
Copy link
Contributor

@Azquelt Azquelt commented Jan 10, 2024

Follow up to #479 to add a few more tests.

  1. Extend InvocationContextTest to include an interceptor binding annotation which is annotated with another interceptor binding.
  2. Test applying an interceptor binding to a method using an extension and then retrieving the binding from the InvocationContext.

Test that additional bindings (where a binding annotation is placed on
another binding annotation) are returned by
InvocationContext.getInterceptorBindings.

Signed-off-by: Andrew Rouse <anrouse@uk.ibm.com>
@Ladicek
Copy link
Contributor

Ladicek commented Jan 10, 2024

LGTM, though I had to think a little about whether transitive interceptor bindings should appear. They probably should, together with inherited interceptor bindings, but it's likely best to make a small amend to the Interceptors spec: jakartaee/interceptors#106

I also have a change locally that adds a test for getInterceptorBindings() in case of:

  • inherited interceptor binding
  • method-level interceptor binding "overriding" class-level interceptor binding

I can either add it to this PR, or submit a separate one.

Test that an interceptor binding added to a method via an annotation is
used to bind the interceptor and visible via
InterceptionContext.getInterceptorBinding

Signed-off-by: Andrew Rouse <anrouse@uk.ibm.com>
@Azquelt
Copy link
Contributor Author

Azquelt commented Jan 10, 2024

I've made the requested changes. I don't mind whether @Ladicek wants to add his tests to this PR or make a new one.

@Ladicek
Copy link
Contributor

Ladicek commented Jan 11, 2024

I'll submit a 2nd PR once we merge this, then. Probably easiest to review that way.

Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

LGTM

@Emily-Jiang
Copy link
Contributor

Emily-Jiang commented Jan 11, 2024

The code changes look good. I have one question regarding copyright. I noticed in most other files, the following copyright text was used:

/*

  • JBoss, Home of Professional Open Source
  • Copyright , Red Hat, Inc., and individual contributors
  • by the @authors tag. See the copyright.txt in the distribution for a
  • full listing of individual contributors.
  • Licensed under the Apache License, Version 2.0 (the "License");
  • you may not use this file except in compliance with the License.
  • You may obtain a copy of the License at
  • http://www.apache.org/licenses/LICENSE-2.0
  • Unless required by applicable law or agreed to in writing, software
  • distributed under the License is distributed on an "AS IS" BASIS,
  • WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  • See the License for the specific language governing permissions and
  • limitations under the License.
    */

In other Jakarta EE projects such as Jakarta Data. The following copyright header is used:

/*

  • Copyright (c) 2023 Contributors to the Eclipse Foundation
  • This program and the accompanying materials are made available under the
  • terms of the Eclipse Public License v. 2.0, which is available at
  • http://www.eclipse.org/legal/epl-2.0.
  • This Source Code may also be made available under the following Secondary
  • Licenses when the conditions for such availability set forth in the
  • Eclipse Public License v. 2.0 are satisfied: GNU General Public License,
  • version 2 with the GNU Classpath Exception, which is available at
  • https://www.gnu.org/software/classpath/license.html.
  • SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
    */

Are there any objections to start using the EF copyright for the new files from now on?

@manovotn
Copy link
Contributor

The code changes look good. I have one question regarding copyright. I noticed in most other files, the following copyright text was used:

These were there historically and as far as I can tell, since the move to Jakarta we haven't been adding them anywhere.

Are there any objections to start using the EF copyright for the new files from now on?

Is there a formal requirement to explicitly add them to all new files? It was my understanding that copyrights on each file are not required. That being said, I'm by no means an expert in this area. It's just my personal take that if we can avoid adding any copyright to files, we should.

@Emily-Jiang
Copy link
Contributor

The code changes look good. I have one question regarding copyright. I noticed in most other files, the following copyright text was used:

These were there historically and as far as I can tell, since the move to Jakarta we haven't been adding them anywhere.

Are there any objections to start using the EF copyright for the new files from now on?

Is there a formal requirement to explicitly add them to all new files? It was my understanding that copyrights on each file are not required. That being said, I'm by no means an expert in this area. It's just my personal take that if we can avoid adding any copyright to files, we should.

@waynebeaton can you confirm?

@Ladicek
Copy link
Contributor

Ladicek commented Jan 11, 2024

I would very much prefer to deal with license headers in a different issue / PR.

I know that by now, we have many files in the TCK that don't have a license header (because I created them :-) ), so if there's a problem, there needs to be a holistic solution.

@Emily-Jiang
Copy link
Contributor

I would very much prefer to deal with license headers in a different issue / PR.

I know that by now, we have many files in the TCK that don't have a license header (because I created them :-) ), so if there's a problem, there needs to be a holistic solution.

We can merge this PR and deal with the header issue later once we hear from Wayne.

@Emily-Jiang Emily-Jiang merged commit b36995b into jakartaee:master Jan 11, 2024
4 checks passed
@waynebeaton
Copy link

Is there a formal requirement to explicitly add them to all new files? It was my understanding that copyrights on each file are not required. That being said, I'm by no means an expert in this area. It's just my personal take that if we can avoid adding any copyright to files, we should.

@waynebeaton can you confirm?

Please add a copyright and license header on every file when the format permits it.

https://www.eclipse.org/projects/handbook/#ip-copyright-headers

@Emily-Jiang
Copy link
Contributor

Is there a formal requirement to explicitly add them to all new files? It was my understanding that copyrights on each file are not required. That being said, I'm by no means an expert in this area. It's just my personal take that if we can avoid adding any copyright to files, we should.

@waynebeaton can you confirm?

Please add a copyright and license header on every file when the format permits it.

https://www.eclipse.org/projects/handbook/#ip-copyright-headers

Thank you @waynebeaton for the confirmation! I have opened this issue to address your concern.

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

5 participants