the original design has: /api/facilities/{facility_id}/cycle-counts/request/batch (though now that I look at it, maybe .../requests/batch is better)

I did it that way to be more explicit about the fact that this is a batch API call. IMO /cycle-counts/requests would be used for simple CRUD APIs on a single CycleCountRequest. But maybe that's just personal preference.

I'm fine with adding /batch.

I'd do createRequests since this is creating multiple

yeah, forgot to change it, I had the same feeling at some point, thanks for pointing that out

I'll let Artur review these since he did the bulk of the work on statuses but do we need both CREATED and REQUESTED?

It's probably worthwhile adding docstrings to each of the statuses to describe what they mean. Some are obvious, but still probably helpful for future us to understand.

I consulted it with Artur before, and we agreed that for now we can go the easiest way possible and elaborate on it either in the code review or refactor it later, since this wouldn't be a difficult change.

I was assuming this would be a CycleCountRequestStatus enum. Won't a CycleCountRequest and CycleCount have separate statuses?

Yeah, if so, I can rename it to CycleCountRequestStatus.

okay now I understand why you didn't make it a batch API call. I thought about doing it this way originally since it avoids needing to define the blindCount for each product, but I ended up designing it with the batch since it was clearer and more flexible IMO.

With the batch it'd be making a POST on a List<CycleCountRequestCommand>:

/api/facilities/{facility_id}/cycle-counts/requests/batch

[
  {
    requestType: "MANUAL_REQUEST",
    blindCount: true,
    productId: "10001",
  },
  ...
],

the main advantage of this is that we can easily make non-batched calls by removing the /batch from the URI and using the same CycleCountRequestCommand object:

/api/facilities/{facility_id}/cycle-counts/requests

{
  requestType: "MANUAL_REQUEST",
  blindCount: true,
  productId: "10001",
}

which IMO makes the most intuitive sense structurally. (It also allows us to optionally configure the "blindCount" value per product, which we likely don't need to do but it's nice to have the option.) Let me know your thoughts

PS, in my original docs I had the batch payload as:

{
  requests: [
    {
      requestType: "MANUAL_REQUEST",
      blindCount: true,
      productId: "10001",
    },
    ...
  ],
}

so it'd have a separate CycleCountRequestBatchCommand object. This way, we can still have the constraints block that you have for verifying things like not having the same product twice in the "requests" field

Actually since this is product based, and the facility, requestType, blindCount remains the same for all products, the way I built it is:

{
    "facility": "8a8a9e9665c4f59d0165c54ec6b10027",
    "products": [
        "10004",
        "10005",
    ],
    "blindCount": true
}

and then I loop through the command.products.
I thought it was redundant to build it like you've just shown, because the only "variable" here is the productId.

Aside:
This was also easier from the technical point of view, because if I had to do it in the way you described, we would need two command classes to make it work, so:

class Command1 {
  List<Command2> command
}

and

class Command2 {
  CycleCountRequestType requestType
  Boolean blindCount
  Product product
}

because we can't bind a command with list type, so something like this:

def createRequests(List<Command2> command)

wouldn't work

yeah that's the tradeoff. I do still prefer having the XCommand and XBatchCommand (which wraps a List) objects since IMO that gives the clearest representation and isolation of the data and the operations we're performing, but I definitely get the appeal of wanting the simpler API that avoids redundant field duplication. Ultimately I'm fine with it the way you have it designed, it just gives us a bit less flexibility in the future if we need to change the API requirements.

(Also, annoying that it doesn't allow you to bind a list, though I believe that's not a recommended practice anyway...)

should this be VARCHAR since it's based on an enum?

you are right, I've just copied the type from the id. Will fix it.

same here. Should be VARCHAR?

yeah you'd want to change the findBy to something like a countByProductAndFacilityAndStatus (where status not in [COMPLETED, CANCELED])

Probably also worth adding a convenience method in the enum to return a list of all "closed" (or all "open") statuses.

yeah, I left that FIXME to remember, as this will be doable after CycleCount domain is created and working.

can't you prevent this in the constraints block of the CycleCountRequestCommand? Loop through all products and check for duplicates. That way we can fail immediately if there are two products provided.

I can handle most of the cases in the validator, and will do it. This line was going a bit ahead of what the typical use cases would be - this line e.g. prevents us from persisting a cycle count request for a product if e.g. two people were performing it at one time, but I guess it's barely to happen so we can live with just validator validation to group products and see if there are any duplicates in the list.

it's an interesting point. We don't want two users creating CCRs for one product at the same time, causing Hibernate to give both the thumbs up (because nothing is in the db yet), and then we get stuck with both creating a CCR successfully for the same product, breaking the system because we have two active requests.

As you mentioned it's not a huge issue for us in our current state since parallel save operations are uncommon, but as we continue to scale up we'll likely see it more often. I think this speaks to a larger problem with how Hibernate propagates the database session across requests. That said, our system as a whole isn't really set up to handle operating at the scale at which it would start being a real problem, and so there are likely many other challenges we'll have to fix first (horizontal scaling with sticky sessions and a load balancer, database scaling and replication...) before this one even becomes a problem for us.

So if we do want to deal with the problem, it'll probably require a deeper investigation as to the best approach to take, and that probably isn't needed for a while (at least I hope). For now I think we can assume it'll be fine.

using productCode causes this to do a lookup on product for each product in the list, correct? I was hoping we could do this validation entirely on productId (which I was assuming would avoid the lookup since we already have the ids in the request body).

But actually this is making me realize I don't know how Grails is binding domains to request/command objects. If we use only productIds, is it smart enough to know not to fetch the products at all?

Product is bound at the binding step, so at this point nothing is fetched. It also doesn't cause any N+1, because productCode is a string field. Potential N+1 would be caused, if I was accessing a collection here.

Aside: I only store and assign the productCode here for a user-friendly validation message, so that the user knows which product it is about (id is not always user-friendly for products, because it's not always id == productCode)

And also I didn't see a point to have the property String productId in the command instead of Product product, because anyway we'd have to fetch the product at some point, and here, Grails does it automatically for us.

this also gives us much flexibility while validating in the constraints and in the validator, because we can already operate on the product and its properties, instead of validating the String property, where the only possibilities would be to validate if it's null/blank.

I was assuming we could bypass fetching the Product at the binding step entirely and just use the id since Product isn't actually used anywhere (except your error case but I'm fine with fetching it there). All we do is map it to the request.

But thinking about it, we do need to verify that the given id is pointing to a real product, so we'll need the lookup regardless. So given we're doing a lookup already, there's no point in not using the Product.

Okay I'm convinced.

you could probably do a countBy productCode (and then filter to > 1) so that you only need to maintain a List (of duplicate productCodes) instead of a map but that's a tiny optimization so I'm not worried about it.

what happens if instead of returning false you return the actual requests.errors? Would that save you from needing to build the errors yourself in the controller?

it's not how the validator works. It expects you either to return true/false (validation passed/didn't pass) or to return a list with a key of an error message and optionally arguments of the message.
Looking for example at a few lines above

 if (!duplicates.isEmpty()) {
  return ['duplicateExists', duplicates.keySet().toString()]
}

this would point to the message:

cycleCountRequestBatchCommand.requests.duplicateExists

and the duplicates.keySet().toString() would be accesible via {3}.
Given that you would have:

return ['duplicateExists', duplicates.keySet().toString(), 'something']

the something would be accesible under {4} etc.
{0}, {1}, {2} are reserved for:

Parameters 0 to 2 are automatically mapped to 0: property name, 1: class name, 2: property value. Additional parameters are mapped starting at parameter 3.

The return false only indicates that there is an error in requests field, so that when you further access the command.errors.requests, it would not be empty.
More info here: https://docs.grails.org/latest/ref/Constraints/validator.html

I see, so you can only return a single failure here but we need to return a list of them. That's annoying.

Okay yes then we'll need to parse the errors as you're doing. Looking through the code, it seems like that's what we've done historically as well (ProductController.batchSave or ProductSupplierAttributeApiController.updateAttributes for example)

(I found a blog post which describes a method of handling this at the validator level, but it feels hacky so I don't think we should do it.)

I've been thinking about how we can standardize our error messages across our system and so maybe there's a way to do this type of "compiling all errors together" work more generically, but for now I'm good with your solution.

Thanks for doing the command object refactoring by the way. I wasn't expecting it to make this validation so messy.

The URI is /api/facilities/{facility_id}/cycle-counts/requests/batch so you could pull the facility value from there instead of requiring it in the request body. Thoughts?

Theoretically I could, but I would lose any validation for facility. Having it here, I don't have to check manually whether it's null or not, because it would be validated out of the box (remember that potentially users might use the api and paste into {facility_id} whatever they want).

that makes sense to me, but then we're completely ignoring the facility in the URI. We could add a check that makes sure the one in the URI matches the one in the request body, but that feels like we're doing redundant work. IMO it's best to have it in a single place to avoid confusion.

And since we want our GET APIs to be able to potentially fetch across facilities (I believe this was Justin's reasoning behind having the "facility" concept instead of using the logged-in location like we do elsewhere), then we'd need it in the URI itself. So because we need it there, I see it as redundant to include it elsewhere.

But I see your point. Having it in the request body gives us automatic validation. However I think if we're going to be including facility in ALL our APIs going forward (which I think is what Justin wants), then maybe we can be smarter about this somehow since all our APIs are going to share some validation on that field.

I'm realizing that we're missing a validation step here to verify that the user has permissions to operate on the requesting facility, so maybe we can have a "getAndValidate(facility, user)" service method on location that gets called early on. Or we could get real funky and create a custom @FacilityAPI (or whatever) annotation that binds the facility for us at the controller level. I'm not sure what's best here since I haven't fully thought it through.

I added beforeValidate clause so that the facility is bound to the one that is provided in the URL.

Was it agreed that it can be skipped now? To add it we will have to write a second migration. Would not it be better if we had the core cycle count domain here as well?

Is the cycle count request meant to be deleted at the end once it is completed? Right now if now if it won't be deleted, then it will prevent to requesting it again in the future due to this constraint. Was there a case when the product is null on the CCR level?

Good question. I would say we would want to keep the history (?), so that we don't delete a request?
As for product nullable, I asked @ewaterman about that, because for me it didn't make sense, and he said we would want to support some other functionalities in the future that you would be able to create a CCR without a product, but probably for a binLocation (?).

yes we never delete a request so this shouldn't be unique. Good catch.

and yes the only reason it's nullable is for future requirements where we can request a count on a location instead of on a product.

"Invalid cycle count request"?

Again the same question, did we want to delete the requests after completing, or should we look here for only PENDING Requests to find duplicates NVM, I just read the TODO, but the other constraint in the CCR domain itself need either improvement or TODO as well.

But I think you could already use the findByProductAndFacilityAndStatusNotInList(product, obj.facility, [COMPLETED, CANCELED])

Yeah, given that you confirmed that, I will add it. I intentionally skipped the statuses part until it is confirmed, but I guess those two statuses are safe bet that they will be here.