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

HttpHeaderInjection to include headers optional modules #939

Closed
wants to merge 5 commits into from

Conversation

whikloj
Copy link
Collaborator

@whikloj whikloj commented Nov 3, 2015

Address FCREPO-1795

@whikloj
Copy link
Collaborator Author

whikloj commented Nov 3, 2015

Needed by fcrepo4/fcrepo-module-auth-webac#48

* @since 2015-10-30
*/
@Component
public class HttpHeaderInjection implements ApplicationContextAware {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a small thing, but wouldn't this read better as HttpHeaderInjector?

Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer subclassing ApplicationObjectSupport to impl'ing ApplicationContextAware.

@whikloj
Copy link
Collaborator Author

whikloj commented Nov 3, 2015

Sorry @ajs6f , I switched back to implementing ApplicationContextAware to avoid the NullPointerExceptions occurring (seemingly) because this only pulls from the external WebAC module currently.

@ajs6f
Copy link
Contributor

ajs6f commented Nov 3, 2015

It's just a plain generic. The bounds don't matter at all. You have one method: T create(final FedoraResource resource, final UriInfo uriInfo, final IdentifierConverter<Resource,FedoraResource> idTranslator) in the factory types (each of which subtypes UriAwareFactory<T>) and one method in the generic supertype to the injectors: UriAwareFactory<T> getUriAwareFactories().

@ajs6f
Copy link
Contributor

ajs6f commented Nov 3, 2015

If you have resolved the injection problems, why not switch back to @Optional? Why are you having NPEs?

@whikloj
Copy link
Collaborator Author

whikloj commented Nov 3, 2015

I did switch back to @Optional, I still get a NPE when it tries to perform the getBeansOfType. I assume because there are none?

As for the generics, would we filter for the type in each utility? Because I am guessing both the HttpTripleUtil and the HttpHeaderInjector would grab each others classes, but we would only want to execute our own to ensure we get the correct output (RDF vs headers).

@ajs6f
Copy link
Contributor

ajs6f commented Nov 3, 2015

No, that shouldn't be giving you an NPE, at least not the way I understand it now. It should work the same way as does the injection of httpTripleUtil.

@ajs6f
Copy link
Contributor

ajs6f commented Nov 3, 2015

You could filter, or I think you could use the type of T available in the superclass.

@whikloj
Copy link
Collaborator Author

whikloj commented Nov 3, 2015

Alright, the NPE is because I was not instantiating the ApplicationObjectSupport correctly. So that is solved.

@ajs6f
Copy link
Contributor

ajs6f commented Nov 3, 2015

MAXIMIZE THE SOLVED!

@whikloj
Copy link
Collaborator Author

whikloj commented Nov 3, 2015

Forgive my ignorance, but if UriAwareHttpHeaderFactory is an interface and UriAwareResourceModelFactory is an interface. Would I generate an "parent" interface with no methods defined?

@ajs6f
Copy link
Contributor

ajs6f commented Nov 3, 2015

No, I'm saying you try to give each factory a supertype, e.g. UriAwareFactory<R>, where R is Model in one case and Multimap<String, String> in the other, and it contains one method R create(final FedoraResource resource, final UriInfo uriInfo, final IdentifierConverter<Resource,FedoraResource> idTranslator) so that createHttpHeadersForResource => @Override Multimap<String, String> create(...) and createModelForResource => @Override Model create(...).

/**
* Given a resource and session, update the JAX-RS response with any additional headers.
*
* @param uriInfo the UriInfo
Copy link

Choose a reason for hiding this comment

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

Please provide even more meaningful javadocs: uriInfo, resource, and return.

@awoods
Copy link

awoods commented Nov 5, 2015

Resolved with: 0977142

@awoods awoods closed this Nov 5, 2015
@awoods awoods deleted the fcrepo-1795 branch November 5, 2015 23:02
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

3 participants