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.

Sign up for GitHub

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

add missing office where clause in Clob getAll query #937

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

add missing office where clause in Clob getAll query #937

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ab2bf61
add missing office where clause in Clob getAll query
adamkorynta Oct 31, 2024
723e38f
remove unused methods
adamkorynta Nov 7, 2024
a9d602f
fix ordering and add integration test
adamkorynta Nov 7, 2024
d6f48b6
update to use office id in the clob cursor
adamkorynta Nov 8, 2024
a0a6097
fix failing unit test
adamkorynta Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions cwms-data-api/src/main/java/cwms/cda/data/dao/AuthDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,11 +138,6 @@ public static AuthDao getInstance(DSLContext dsl) {
return getInstance(dsl, null);
}

@Override
public List<DataApiPrincipal> getAll(String limitToOffice) {
throw new UnsupportedOperationException("Unimplemented method 'getAll'");
}

rma-rripken marked this conversation as resolved.
Show resolved Hide resolved
/**
* Reserved for future use, get user principal by presented unique name and office.
* (Also required by Dao&lt;DataApiPrincipal&gt;)
Expand Down
27 changes: 0 additions & 27 deletions cwms-data-api/src/main/java/cwms/cda/data/dao/BlobDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,33 +110,6 @@ private static void handleResultSet(ResultSet resultSet, BlobConsumer consumer)
consumer.accept(blob, mediaType);
}


@Override
public List<Blob> getAll(String officeId) {
String queryStr = "SELECT AT_BLOB.ID, AT_BLOB.DESCRIPTION, CWMS_MEDIA_TYPE.MEDIA_TYPE_ID, CWMS_OFFICE.OFFICE_ID\n"
+ " FROM CWMS_20.AT_BLOB \n"
+ "join CWMS_20.CWMS_MEDIA_TYPE on AT_BLOB.MEDIA_TYPE_CODE = CWMS_MEDIA_TYPE.MEDIA_TYPE_CODE \n"
+ "join CWMS_20.CWMS_OFFICE on AT_BLOB.OFFICE_CODE=CWMS_OFFICE.OFFICE_CODE \n"
;

ResultQuery<Record> query;
if (officeId != null) {
queryStr = queryStr + " and upper(CWMS_OFFICE.OFFICE_ID) = upper(?)";
query = dsl.resultQuery(queryStr, officeId);
} else {
query = dsl.resultQuery(queryStr);
}

return query.fetch(r -> {
String rId = r.get("ID", String.class);
String rOffice = r.get("OFFICE_ID", String.class);
String rDesc = r.get("DESCRIPTION", String.class);
String rMedia = r.get("MEDIA_TYPE_ID", String.class);

return new Blob(rOffice, rId, rDesc, rMedia, null);
});
}

public List<Blob> getAll(String officeId, String like) {
String queryStr = "SELECT AT_BLOB.ID, AT_BLOB.DESCRIPTION, CWMS_MEDIA_TYPE.MEDIA_TYPE_ID, CWMS_OFFICE.OFFICE_ID\n"
+ " FROM CWMS_20.AT_BLOB \n"
Expand Down
81 changes: 21 additions & 60 deletions cwms-data-api/src/main/java/cwms/cda/data/dao/ClobDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,29 +42,6 @@ public ClobDao(DSLContext dsl) {
super(dsl);
}


// Yikes, I hate this method - it retrieves all the clobs? That could be gigabytes of data.
// Not returning Value or Desc fields until a useful way of working with this method is
// figured out.
@Override
public List<Clob> getAll(String limitToOffice) {
AV_CLOB ac = AV_CLOB.AV_CLOB;
AV_OFFICE ao = AV_OFFICE.AV_OFFICE;

Condition whereCond = noCondition();
if (limitToOffice != null && !limitToOffice.isEmpty()) {
whereCond = ao.OFFICE_ID.eq(limitToOffice);
}

return dsl.select(ac.ID, ao.OFFICE_ID)
.from(ac.join(ao).on(ac.OFFICE_CODE.eq(ao.OFFICE_CODE)))
.where(whereCond)
.fetch(joinRecord ->
new Clob(joinRecord.get(ao.OFFICE_ID),
joinRecord.get(ac.ID), null, null));

}

@Override
public Optional<Clob> getByUniqueName(String uniqueName, String office) {
AV_CLOB ac = AV_CLOB.AV_CLOB;
Expand All @@ -88,28 +65,24 @@ public Optional<Clob> getByUniqueName(String uniqueName, String office) {
.fetchOptional(mapper);
}

public Clobs getClobs(String cursor, int pageSize, String officeLike,
boolean includeValues) {
return getClobs(cursor, pageSize, officeLike, includeValues, ".*");
}

public Clobs getClobs(String cursor, int pageSize, String officeLike,
boolean includeValues, String idRegex) {
int total = 0;
String clobCursor = "*";
AV_CLOB v_clob = AV_CLOB.AV_CLOB;
AV_OFFICE v_office = AV_OFFICE.AV_OFFICE;

Condition whereClause = JooqDao.caseInsensitiveLikeRegex(v_clob.ID, idRegex)
.and(officeLike == null ? noCondition() : JooqDao.caseInsensitiveLikeRegex(v_office.OFFICE_ID, officeLike));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't you add a caseInsensitiveLikeRegexNullTrue that does exactly this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wow, I have no recollection of writing that, but I guess I did!

if (cursor == null || cursor.isEmpty()) {

SelectConditionStep<Record1<Integer>> count =
dsl.select(count(asterisk()))
.from(v_clob)
.join(v_office).on(v_clob.OFFICE_CODE.eq(v_office.OFFICE_CODE))
.where(JooqDao.caseInsensitiveLikeRegex(v_clob.ID, idRegex))
.and(officeLike == null ? noCondition() : DSL.upper(v_office.OFFICE_ID).like(officeLike.toUpperCase()));

total = count.fetchOne().value1();
SelectConditionStep<Record1<Integer>> count = dsl.select(count(asterisk()))
.from(v_clob)
.join(v_office).on(v_clob.OFFICE_CODE.eq(v_office.OFFICE_CODE))
.where(whereClause);
Record1<Integer> rec = count.fetchOne();
if(rec != null) {
total = rec.value1();
}
} else {
final String[] parts = CwmsDTOPaginated.decodeCursor(cursor, "||");

Expand All @@ -128,17 +101,17 @@ public Clobs getClobs(String cursor, int pageSize, String officeLike,
}

SelectLimitPercentStep<Record4<String, String, String, String>> query = dsl.select(
v_office.OFFICE_ID,
v_clob.ID,
v_clob.DESCRIPTION,
includeValues ? v_clob.VALUE : DSL.inline("").as(v_clob.VALUE)
)
.from(v_clob)
//.innerJoin(forLimit).on(forLimit.field(v_clob.ID).eq(v_clob.ID))
.join(v_office).on(v_clob.OFFICE_CODE.eq(v_office.OFFICE_CODE))
.where(JooqDao.caseInsensitiveLikeRegex(v_clob.ID,idRegex))
.and(DSL.upper(v_clob.ID).greaterThan(clobCursor))
.orderBy(v_clob.ID).limit(pageSize);
v_office.OFFICE_ID,
v_clob.ID,
v_clob.DESCRIPTION,
includeValues ? v_clob.VALUE : DSL.inline("").as(v_clob.VALUE)
)
.from(v_clob)
.join(v_office).on(v_clob.OFFICE_CODE.eq(v_office.OFFICE_CODE))
.where(whereClause)
.and(DSL.upper(v_clob.ID).greaterThan(clobCursor))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This still isn't quite right. At least in my opinion. If you are ordering by office and clobId then I think the paging has to do the sameOffice/nextOffice thing. See buildPagingCondition in TimeSeriesDaoImpl for what I'm talking about

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fairly certain Ryan is correct. especially if you have more than one office possibly coming through, but even just limiting to a single you want that sorting to be consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup, thanks for catching this. Updated to match how you handled it in the projects endpoint

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there another commit? I don't see it in these changes

.orderBy(v_office.OFFICE_ID, v_clob.ID)
.limit(pageSize);


Clobs.Builder builder = new Clobs.Builder(clobCursor, pageSize, total);
Expand Down Expand Up @@ -181,18 +154,6 @@ public List<Clob> getClobsLike(String office, String idLike) {
ac.join(ao).on(ac.OFFICE_CODE.eq(ao.OFFICE_CODE))).where(cond).fetch(mapper);
}

public String getClobValue(String office, String id) {
AV_CLOB ac = AV_CLOB.AV_CLOB;
AV_OFFICE ao = AV_OFFICE.AV_OFFICE;

Condition cond = ac.ID.eq(id).and(ao.OFFICE_ID.eq(office));

Record1<String> clobRecord = dsl.select(ac.VALUE).from(
ac.join(ao).on(ac.OFFICE_CODE.eq(ao.OFFICE_CODE))).where(cond).fetchOne();

return clobRecord.value1();
}

public void create(Clob clob, boolean failIfExists) {

String pFailIfExists = getBoolean(failIfExists);
Expand Down
3 changes: 0 additions & 3 deletions cwms-data-api/src/main/java/cwms/cda/data/dao/Dao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,6 @@ protected void setOffice(Connection c, String office) throws SQLException {
CWMS_ENV_PACKAGE.call_SET_SESSION_OFFICE_ID(DSL.using(c).configuration(), office);
}


public abstract List<T> getAll(String office);

public abstract Optional<T> getByUniqueName(String uniqueName, String office);

}
5 changes: 0 additions & 5 deletions cwms-data-api/src/main/java/cwms/cda/data/dao/JooqDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,11 +155,6 @@ private static Connection setClientInfo(Context ctx, Connection connection) {
return connection;
}

@Override
public List<T> getAll(String officeId) {
throw new UnsupportedOperationException("Not supported yet.");
}

@Override
public Optional<T> getByUniqueName(String uniqueName, String officeId) {
throw new UnsupportedOperationException("Not supported yet.");
Expand Down
53 changes: 50 additions & 3 deletions cwms-data-api/src/test/java/cwms/cda/api/ClobControllerTestIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package cwms.cda.api;

import static io.restassured.RestAssured.given;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.is;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import cwms.cda.data.dto.Clob;
import cwms.cda.formatters.Formats;
Expand All @@ -13,7 +13,6 @@
import java.io.UnsupportedEncodingException;
import java.net.URLEncoder;
import javax.servlet.http.HttpServletResponse;

import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
Expand All @@ -29,7 +28,7 @@ public class ClobControllerTestIT extends DataApiTestIT {
private static final String EXISTING_CLOB_DESC = "test description";

@BeforeAll
static void createExistingClob() throws Exception
static void createExistingClobs() throws Exception
{
Clob clob = new Clob(SPK, EXISTING_CLOB_ID, EXISTING_CLOB_DESC, EXISTING_CLOB_VALUE);
ObjectMapper om = JsonV2.buildObjectMapper();
Expand All @@ -51,6 +50,26 @@ static void createExistingClob() throws Exception
.log().ifValidationFails(LogDetail.ALL,true)
.assertThat()
.statusCode(is(HttpServletResponse.SC_CREATED));
//Need to verify that getAll filters to a specific office
user = TestAccounts.KeyUser.SWT_NORMAL;
clob = new Clob(user.getOperatingOffice(), EXISTING_CLOB_ID, EXISTING_CLOB_DESC, EXISTING_CLOB_VALUE);
serializedClob = om.writeValueAsString(clob);
given()
.log().ifValidationFails(LogDetail.ALL,true)
.accept(Formats.JSONV2)
.contentType(Formats.JSONV2)
.body(serializedClob)
.header("Authorization",user.toHeaderValue())
.queryParam("office",SPK)
.queryParam("fail-if-exists",false)
.when()
.redirects().follow(true)
.redirects().max(3)
.post("/clobs/")
.then()
.log().ifValidationFails(LogDetail.ALL,true)
.assertThat()
.statusCode(is(HttpServletResponse.SC_CREATED));
}

@Test
Expand Down Expand Up @@ -131,6 +150,34 @@ void test_getOne_plain_text()
.body( is(EXISTING_CLOB_VALUE));
}

@Test
void test_getAll_specific_office()
{
given()
.log().ifValidationFails(LogDetail.ALL,true)
.queryParam(Controllers.OFFICE, SPK)
.accept(Formats.JSON)
.when()
.get("/clobs/")
.then()
.log().ifValidationFails(LogDetail.ALL,true)
.assertThat()
.statusCode(is(HttpServletResponse.SC_OK))
.body("clobs.office-id", hasItem(SPK));

given()
.log().ifValidationFails(LogDetail.ALL,true)
.queryParam(Controllers.OFFICE, TestAccounts.KeyUser.SWT_NORMAL.getOperatingOffice())
.accept(Formats.JSON)
.when()
.get("/clobs/")
.then()
.log().ifValidationFails(LogDetail.ALL,true)
.assertThat()
.statusCode(is(HttpServletResponse.SC_OK))
.body("clobs.office-id", hasItem(TestAccounts.KeyUser.SWT_NORMAL.getOperatingOffice()));
}


@ParameterizedTest
@EnumSource(GetAllTest.class)
Expand Down
Loading