Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

NXCM-4861: Fixing the ArtifactStoreHelper.storeItemWithChecksums method #706

Merged
merged 3 commits into from Jan 4, 2013

Conversation

Projects
None yet
3 participants
Contributor

cstamas commented Dec 18, 2012

In some cases, current imple can result in remote item download
for some proxy configuration, like itemAgingActive=false used by
procurement.

Fixing it, to enforce "localOnly" flag. Basically, same code
is now executed as in case of cache (where also store+retrieve)
happens, see:
org.sonatype.nexus.proxy.repository.AbstractProxyRepository#doCacheItem

Other minor cleanup (mostly javadoc) done.

Added UT that verifies that maven repo now receives a retrieve
with localOnly=true flag.

@cstamas cstamas NXCM-4861: Fixing the ArtifactStoreHelper.storeItemWithChecksums method
In some cases, current imple can result in remote item download
for some proxy configuration, like itemAgingActive=false used by
procurement.

Fixing it, to enforce "localOnly" flag. Basically, same code
is now executed as in case of cache (where also store+retrieve)
happens, see:
org.sonatype.nexus.proxy.repository.AbstractProxyRepository#doCacheItem

Other minor cleanup (mostly javadoc) done.

Added UT that verifies that maven repo now receives a retrieve
with localOnly=true flag.
de1d368

@ifedorenko ifedorenko commented on an outdated diff Jan 4, 2013

...java/org/sonatype/nexus/proxy/maven/Nxcm4861Test.java
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.sonatype.configuration.ConfigurationException;
+import org.sonatype.nexus.configuration.model.CLocalStorage;
+import org.sonatype.nexus.configuration.model.CRepository;
+import org.sonatype.nexus.configuration.model.DefaultCRepository;
+import org.sonatype.nexus.proxy.AbstractProxyTestEnvironment;
+import org.sonatype.nexus.proxy.EnvironmentBuilder;
+import org.sonatype.nexus.proxy.ResourceStoreRequest;
+import org.sonatype.nexus.proxy.maven.maven2.M2Repository;
+import org.sonatype.nexus.proxy.maven.maven2.M2RepositoryConfiguration;
+import org.sonatype.nexus.proxy.repository.Repository;
+import org.sonatype.nexus.proxy.storage.local.LocalRepositoryStorage;
+import org.sonatype.sisu.litmus.testsupport.mock.MockitoRule;
+
+public class Nxcm4861Test
@ifedorenko

ifedorenko Jan 4, 2013

Contributor

This needs more descriptive name, that tells what the test actually tests, something like Nxcm4861ArtifactStoreHelper_storeItemWithChecksumsTest

@peterlynch peterlynch commented on an outdated diff Jan 4, 2013

...ain/java/org/sonatype/nexus/proxy/RequestContext.java
@@ -30,13 +30,13 @@
/** Context URL of the original resource requested on the incoming connector. */
public static final String CTX_REQUEST_URL = "request.url";
- /** Context flag to mark a request local only. */
+ /** Context flag to mark a request local only (proxy: do not attempt remote access at all, else: no effect). */
public static final String CTX_LOCAL_ONLY_FLAG = "request.localOnly";
@peterlynch

peterlynch Jan 4, 2013

Member

since this is public javadoc, why not link directly to ProxyRepository interface via link gives relevant context plus aids IDE refactoring and and context

@peterlynch peterlynch commented on an outdated diff Jan 4, 2013

...ain/java/org/sonatype/nexus/proxy/RequestContext.java
public static final String CTX_LOCAL_ONLY_FLAG = "request.localOnly";
- /** Context flag to mark a request local only. */
+ /** Context flag to mark a request local only (proxy: force remote access -- might still serve local if up2date, else: no effect). */
@peterlynch

peterlynch Jan 4, 2013

Member

same javadoc suggestion - reference 'up2date' variable, method or something - ie. more detail

Also should be "Context flag to mark a remote request local only"

@peterlynch peterlynch commented on an outdated diff Jan 4, 2013

...ain/java/org/sonatype/nexus/proxy/RequestContext.java
public static final String CTX_REMOTE_ONLY_FLAG = "request.remoteOnly";
- /** Context flag to mark a request local only. */
+ /** Context flag to mark a request local only (group: do not "dive" into members, else: no effect). */
@peterlynch

peterlynch Jan 4, 2013

Member

would link to GroupRepository interface, not just use 'group'

Also should be "Context flag to mark a group request local only"

@peterlynch peterlynch commented on an outdated diff Jan 4, 2013

...va/org/sonatype/nexus/proxy/ResourceStoreRequest.java
+ /**
+ * Constructor.
+ *
+ * @param requestPath
+ * @param localOnly
+ * @param remoteOnly
@peterlynch

peterlynch Jan 4, 2013

Member

What's the point of the javadoc if you do not describe what the params are for

@peterlynch peterlynch commented on the diff Jan 4, 2013

...va/org/sonatype/nexus/proxy/ResourceStoreRequest.java
@@ -349,10 +375,12 @@ public void addAppliedMappingsList( Repository repository, List<String> mappingL
@Override
public String toString()
{
- StringBuilder sb = new StringBuilder( getClass().getSimpleName() );
- sb.append( "(requestPath=\"" );
- sb.append( getRequestPath() );
- sb.append( "\")" );
- return sb.toString();
+ return "ResourceStoreRequest{" +
+ "requestPath='" + requestPath + '\'' +
@peterlynch

peterlynch Jan 4, 2013

Member

why add the artificial slash here? to request path

...and before it was a leading slash, now it is added at end.

@cstamas cstamas added a commit that referenced this pull request Jan 4, 2013

@cstamas cstamas Merge pull request #706 from sonatype/nxcm-4861
NXCM-4861: Fixing the ArtifactStoreHelper.storeItemWithChecksums method
1b147dd

@cstamas cstamas merged commit 1b147dd into master Jan 4, 2013

@cstamas cstamas deleted the nxcm-4861 branch Jan 4, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment